Skip to content
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.

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

lib.strings: add trim #315411

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

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 :)

lib/tests/misc.nix Outdated Show resolved Hide resolved
lib/strings.nix Outdated Show resolved Hide resolved
lib/strings.nix Outdated Show resolved Hide resolved
@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 labels May 28, 2024
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 29, 2024
@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented May 29, 2024

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 reputable person label May 31, 2024
@AndersonTorres
Copy link
Member

AndersonTorres commented Jun 2, 2024

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".

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Jul 24, 2024

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".

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

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

lib/strings.nix Outdated Show resolved Hide resolved
@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

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?

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!

lib/strings.nix Outdated Show resolved Hide resolved
lib/strings.nix Outdated Show resolved Hide resolved
`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.
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
12 of 13 checks passed
@MattSturgeon MattSturgeon deleted the trim branch July 25, 2024 23:15
@infinisil
Copy link
Member

Small follow-up: #330034

@2xsaiko 2xsaiko added the backport release-24.05 Backport PR automatically label Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Successfully created backport PR for release-24.05:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 backport release-24.05 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants