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.

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

lib/types: enhances separatedString's description. #48251

Merged
merged 1 commit into from Oct 12, 2018
Merged

lib/types: enhances separatedString's description. #48251

merged 1 commit into from Oct 12, 2018

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Oct 12, 2018 •
edited

The previous description "string" is misleading in the full options
manual pages; they are actually concatenated strings, with a specific
character.

The string "" has been documented to stay coherent with the previously
deprecated "types.string".

Motivation for this change

This is for nixos-homepage/pull/246, where @ryantm was curious about listing the type descriptions on the options page. The particular type desired had to be fixed to be more useful (I think).

To be useful for nixos-homepage, it would need to be backported to 18.09.

Things done
  • ✔️ Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • ✔️ NixOS
    • ⬜ macOS
    • ⬜ other Linux distributions
  • ⬜ Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • ⬜ Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • ⬜ Tested execution of all binary files (usually in ./result/bin/)
  • ⬜ Determined the impact on package closure size (by running nix path-info -S before and after)
  • ✔️ Fits CONTRIBUTING.md.
  • ✔️ Built the docs

@samueldr samueldr mentioned this pull request Oct 12, 2018
Merged
ryantm
ryantm requested changes Oct 12, 2018
View reviewed changes
lib/types.nix Outdated
@@ -194,7 +194,10 @@ rec {
# separator between the values).
separatedString = sep: mkOptionType rec {
name = "separatedString";
description = "string";
description = if sep == ""
then "string" # for types.string.
Copy link
Member

Choose a reason for hiding this comment

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

This description is still misleading because it is the same description as lib.types.str, which is the preferred (non-deprecated) string type. I think the else branch can work for all cases! Should we put \" around the separator?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/NixOS/nixpkgs/blob/master/lib/types.nix#L181

Copy link
Member Author

@samueldr samueldr Oct 12, 2018 •
edited

Choose a reason for hiding this comment

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

I know the description for types.string is misleading: I do not know what it should be. I'm thinking "String concatenated with ''" is too clunky. Maybe "Concatenated string" would be better.

The separator is JSON-encoded, which also adds quotes around the string.

(nix-repl adds more escaping)

nix-repl> builtins.toJSON ""
"\"\""
nix-repl> builtins.toJSON "\n"
"\"\\n\""

Copy link
Member

Choose a reason for hiding this comment

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

What about "Concatenated string (deprecated)"?

Copy link
Member

Choose a reason for hiding this comment

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

@ryantm The fact that it's deprecated shouldn't be part of the description, which might end up in the docs or so.

I like "Concatenated string" for the sep == "" case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated without the "deprecated" bit; its deprecation isn't really a part of its description imho (even though it is deprecated). The end-user, using the module, cannot do anything about the deprecation notice there.

@samueldr samueldr force-pushed the feature/separated-string-description branch from 0fb4190 to d636625 Compare October 12, 2018 20:59
@ryantm
Copy link
Member

ryantm commented Oct 12, 2018

@samueldr Looks good! Your nice commit message isn't quite right anymore.

@samueldr
lib/types: enhances separatedString's description
0808c7c 
The previous description "string" is misleading in the full options
manual pages; they are actually concatenated strings, with a specific
character.

The empty string version ("types.string") has been special-cased to
provide a better message.
@samueldr samueldr force-pushed the feature/separated-string-description branch from d636625 to 0808c7c Compare October 12, 2018 23:12
@samueldr
Copy link
Member Author

Thanks, missed that bit!

@ryantm
Copy link
Member

ryantm commented Oct 12, 2018

@GrahamcOfBorg test nghttpx

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: tests.nghttpx

Partial log (click to expand)

proxy: running command: sync
proxy: exit status 0
test script finished in 18.43s
cleaning up
killing webserver (pid 597)
killing client (pid 609)
killing proxy (pid 621)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/gqp7r01zv3asd40g1d7gcibf5vrzskk7-vm-test-run-nghttpx

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: tests.nghttpx

Partial log (click to expand)

webserver: running command: sync
webserver: exit status 0
test script finished in 89.45s
cleaning up
killing proxy (pid 631)
killing client (pid 643)
killing webserver (pid 657)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/gzakfih1cn93gfkcskk3k5l7yq9h7154-vm-test-run-nghttpx

@ryantm ryantm merged commit 1bde5ec into NixOS:master Oct 12, 2018
@samueldr
Copy link
Member Author

Backported

[release-18.09 03bf8cd] lib/types: enhances separatedString's description
Date: Thu Oct 11 21:23:11 2018 -0400
1 file changed, 4 insertions(+), 1 deletion(-)

@samueldr samueldr deleted the feature/separated-string-description branch February 12, 2019 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil left review comments

@ryantm ryantm ryantm approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@samueldr @ryantm @GrahamcOfBorg @infinisil