-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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: introduce types.hexStr #301039
lib: introduce types.hexStr #301039
Conversation
405916e
to
635213f
Compare
@infinisil I added tests and updated the docs, as requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks quite useful, but I don't think it's a number.
@@ -160,6 +160,10 @@ merging is handled. | |||
may result in [precision loss](https://github.com/NixOS/nix/issues/5733). | |||
::: | |||
|
|||
`types.hex` | |||
|
|||
: A hexadecimal number. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there's a lot more to say, and maybe move it into the section with string-like types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more info, let me know what you think :D
635213f
to
7dec928
Compare
This change introduces lib.types.hex, I thought that it's a common enough numer that it's worth the change. I decided against adding it to other lib functions like lib.types.number, so the interface keeps stable.
7dec928
to
a8eb148
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be a bit more clear about what's represented.
Half bytes seem like a mistake we should catch.
Sorry to drag this out, but it's important for types to have a clear meaning and to be precise about it.
Types are of such a nature that all the details will be depended on rather soon, and not just here in the nixpkgs
repo. I'm not trying to be obsessive for no good reason :)
|
||
: A hexadecimal value represented as string. The type only allows lowercase | ||
values. The type also does not allow for hex prefixes like `0x` or `#`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leading zeroes are not stripped. |
@@ -373,6 +373,14 @@ rec { | |||
merge = mergeEqualOption; | |||
}; | |||
|
|||
hexStr = mkOptionType { | |||
name = "hexStr"; | |||
description = "hexadecimal value represented as string"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the hexadecimal is still just a representation of something, presumably bytes.
description = "hexadecimal value represented as string"; | |
description = "bytes represented as a string of hexadecimals"; |
That reminds me, shouldn't we check that it's an even number of hex digits?
Half bytes would be rare. The only case I can think of is short 12-bit hex color codes of three hex digits, but that should just be a distinct type (and probably a type that also allows 24-bit color codes, and a #
).
description = "hexadecimal value represented as string"; | |
description = "string of hexadecimal bytes"; |
Maybe that's a bit vague. For a more syntax oriented description:
description = "hexadecimal value represented as string"; | |
description = "string of hexadecimal pairs"; |
Or if I'm wrong about half bytes:
description = "hexadecimal value represented as string"; | |
description = "string of hexadecimal digits"; |
description = "hexadecimal value represented as string"; | |
description = "string of hexadecimals"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line with #301039 (comment):
description = "hexadecimal value represented as string"; | |
description = "string of hexadecimal digits"; |
@@ -202,6 +202,12 @@ merging is handled. | |||
|
|||
: A string. Multiple definitions are concatenated with a colon `":"`. | |||
|
|||
`types.hexStr` | |||
|
|||
: A hexadecimal value represented as string. The type only allows lowercase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: A hexadecimal value represented as string. The type only allows lowercase | |
: A sequence of bytes represented as a string of hexadecimals. The type only allows lowercase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half bytes seem like a mistake we should catch.
Uh, I think we might be talking past each other. I this is a string of hex (as in characters representing hex numbers) and not a byte sequence of hex data. So you don't really have half bytes 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did make somewhat of a leap, and I'm sorry if my presentation was messy. I'm assuming half bytes are not ok, and I think the type should check that.
Do you have a use case where the string of hexadecimal digits does not represent bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would be beneficial to talk about half-bytes here. Just saying hexadecimal sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree half bytes are unnecessarily awkward, and I suspect they should not be allowed. If we don't allow them, we can call this a sequence of bytes in hexadecimal; no silly language, and no silly values.
If we do have a use case for odd numbers of hex digits with leading zeroes, then something like "hexadecimal string" sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe I wasn't communicating well, because I was trying to comment about the implementation, but my suggestions are on lines about documentation.
I like to use file comments because they become threads, but it seems that I need to take into account better where I put those comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also only getting it now: You're hinting at wanting a check to prevent against an odd number of hex digits (which would represent half-bytes), which is most likely just a bug. I've personally never encountered any programs asking for an odd number of hex digits (because indeed, hex digits are usually used to represent bytes).
I think it's fair to restrict this type to an even number of hex digits to prevent such mistakes. We can still allow an odd number of digits later if this turns out that there are use cases for an odd number of digits. Because of that though I wouldn't want this to be embedded into the documentation. So I think something like this should be good:
: A hexadecimal value represented as string. The type only allows lowercase | |
: A string of hexadecimal digits. The type only allows lowercase |
name = "hexStr"; | ||
description = "hexadecimal value represented as string"; | ||
descriptionClass = "noun"; | ||
check = x: str.check x && builtins.match "[0-9a-f]+" x != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line with #301039 (comment), something like this:
check = x: str.check x && builtins.match "[0-9a-f]+" x != null; | |
check = x: | |
str.check x | |
&& builtins.match "[0-9a-f]+" x != null | |
# Usually hex digits are used to represent bytes. | |
# A single hex digit only represents half a byte, | |
# so if we have an odd number, it's likely a mistake. | |
&& mod (stringLength x) 2 == 0; |
Writing this, I'm actually not sure if it's worth doing this, the error message when you get the type wrong is really not very helpful in such a case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this only outputs a bool.
What if we check it in the merge function instead, like the other, good errors?
Technically that breaks either hexStr x
for odd-length definitions, but that is probably ok in practice.
Perhaps the module system could invoke the merge function instead of producing a generic error message? Or falling back to the generic message if the merge evaluates (if ! t.check v then seq merged (throw genericMsg) else merged
).
(Just an idea, not a request)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the proper solution is to extend .check
to allow custom error messages.
To move forward with this PR though, I don't really care about having this check or not. I'd like to leave this decision up to @Janik-Haag.
I quit nixpkgs and thus won't be finishing this. |
All the best to you Janik, and thank you for all your contributions. |
Description of changes
This change introduces lib.types.hex, I thought that it's a common enough numer that it's worth the change.
I decided against adding it to other lib functions like lib.types.number, so the interface keeps stable.
not quite sure if there is anything else that needs to be done before this can get merged, or if there are any tests for lib.types I should be aware of.
I also tested if everything is working with the small repl example below
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.