Skip to content

lib.strings: add trim #315411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 25, 2024
Merged

lib.strings: add trim #315411

merged 1 commit into from
Jul 25, 2024

Conversation

MattSturgeon
Copy link
Contributor

@MattSturgeon MattSturgeon commented May 28, 2024 •
edited
Loading

strings.trim returns a copy of the string with all leading and trailing whitespace removed.

strings.trimWith does the same thing, but calling code can decide whether to trim the start and/or end of the string.

The obvious regex [[:string:]]*(.*)[[:string:]]* wouldn't work, because the (.*) is greedy and matches trailing whitespace. Since ERE doesn't support lazy/reluctant matches (.*?) introduced in perl-style regexes, we need to use a regex that fails to match blank/empty strings. This case is handled using optionalString.

Description of changes

trim is a fairly core function included in most standard libraries. I've personally come across a couple scenarios where I would've liked to have been able to trim strings.

I've added a few basic tests.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Pinging recent contributors to lib.string: @infinisil @AndersonTorres @FireyFly @eclairevoyant @h7x4

Add a 👍 reaction to pull requests you find important.

@MattSturgeon MattSturgeon requested a review from infinisil as a code owner May 28, 2024 17:29
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label May 28, 2024
@eclairevoyant
Copy link
Contributor

Should this be added to the top-level (lib/default.nix) as well? (Example PR: #214021)

@MattSturgeon
Copy link
Contributor Author

Should this be added to the top-level (lib/default.nix) as well? (Example PR: #214021)

Perhaps, but based on #266103 I'd guess it's preferred not to add new functions to the top-level?

@infinisil
Copy link
Member

I'll try to review this a bit more closely soon, but yeah for now it should still be exported under lib.* too, because that's the status-quo :)

eclairevoyant
eclairevoyant previously requested changes May 28, 2024
View reviewed changes
infinisil
infinisil previously requested changes May 28, 2024
View reviewed changes
@eclairevoyant eclairevoyant dismissed their stale review May 28, 2024 20:30

changes addressed

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels May 28, 2024
@MattSturgeon MattSturgeon mentioned this pull request May 28, 2024
Merged
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 29, 2024
@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented May 29, 2024 •
edited
Loading

Would nixpkgs be open to also having trimStart and trimEnd (possible aliases/alternate names trimHead/trimTail, trimLeading/trimTrailing)?

If so, would you want them in this PR or separately?

@MattSturgeon MattSturgeon requested a review from infinisil May 30, 2024 02:52
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label May 31, 2024
@AndersonTorres
Copy link
Member

AndersonTorres commented Jun 2, 2024 •
edited
Loading

What is the prevalence of trim to begin with?

In the sense of: "all these lines of code in Nixpkgs are mere applications of trim".

@MattSturgeon MattSturgeon mentioned this pull request Jul 19, 2024
Merged
@eclairevoyant
Copy link
Contributor

eclairevoyant commented Jul 24, 2024 •
edited
Loading

What is the prevalence of trim to begin with?

In the sense of: "all these lines of code in Nixpkgs are mere applications of trim".

nixpkgs/lib/strings.nix

Line 1060 in 4eb44db

# RegEx: Match any leading whitespace, possibly a '-', one or more digits,
toInt and friends all try to implement trim

nixpkgs/lib/strings.nix

Line 1174 in 4eb44db

fileContents = file: removeSuffix "\n" (readFile file);
is more of a trimEnd, but still

Also, I'd want to trim option descriptions in some cases (especially for mkEnableOption).

@eclairevoyant eclairevoyant dismissed infinisil’s stale review July 24, 2024 04:54

changes addressed

AndersonTorres
AndersonTorres reviewed Jul 24, 2024
View reviewed changes
@MattSturgeon MattSturgeon force-pushed the trim branch 2 times, most recently from 7561b86 to 98cb779 Compare July 25, 2024 08:49
@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented Jul 25, 2024 •
edited
Loading

As suggested (#315411 (comment)) I've implemented a configurable trim' trimWith too.

@AndersonTorres does this achieve what you wanted?

@eclairevoyant would you mind re-reviewing since the implementation has changed?

infinisil
infinisil reviewed Jul 25, 2024
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment, otherwise I think this is good!

infinisil
infinisil reviewed Jul 25, 2024
View reviewed changes
@MattSturgeon
lib.strings: add trim and trimWith
aad87c2 
`strings.trim` returns a copy of the string with all leading and trailing
whitespace removed.

`strings.trimWith` does the same thing, but calling code can decide
whether to trim the start and/or end of the string.
@MattSturgeon MattSturgeon requested a review from infinisil July 25, 2024 23:09
infinisil
infinisil approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me, nice work!

@infinisil infinisil merged commit 91a3ba9 into NixOS:master Jul 25, 2024
9 of 10 checks passed
@MattSturgeon MattSturgeon deleted the trim branch July 25, 2024 23:15
@infinisil infinisil mentioned this pull request Jul 25, 2024
Merged
@infinisil
Copy link
Member

Small follow-up: #330034

@infinisil infinisil mentioned this pull request Jul 25, 2024
Merged
13 tasks
@MattSturgeon MattSturgeon mentioned this pull request Oct 3, 2024
Merged
13 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Successfully created backport PR for release-24.05:

@github-actions github-actions bot mentioned this pull request Nov 7, 2024
Merged
1 task
@khaneliman khaneliman mentioned this pull request May 19, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil approved these changes

@eclairevoyant eclairevoyant Awaiting requested review from eclairevoyant

+1 more reviewer

@AndersonTorres AndersonTorres AndersonTorres approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MattSturgeon @eclairevoyant @infinisil @AndersonTorres @dblsaiko @wegank