feat: hints on slices #263
Conversation
|
letFunny
left a comment
letFunny
left a comment
There was a problem hiding this comment.
Thank you for working on this Paul and relentlessly discussing all the details of the UX until we are satisfied with a good convention. I added some comments, mostly minor, please tell me what you think.
internal/setup/setup_test.go
Outdated
| /usr/bin/cc: | ||
| `, | ||
| }, | ||
| relerror: `invalid slice hint for "slice1" in slices/mydir/mypkg.yaml: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" \(len <= 40, no line breaks\)`, |
There was a problem hiding this comment.
If the value can be very long or contain multiple lines, sometimes it helps to put the condition \(len <= 40 ... before the value so that the user doesn't have to parse the whole error and can instead see what failed from left to right skipping the input at the end.
There was a problem hiding this comment.
Good point. I will fix it but out of consistency I am also tempted to change the error message on the slice name (even if the risk of a long error is lower) so they both look the same. WDYT?
There was a problem hiding this comment.
I am on the fence on this one. I value consistency a lot* but I also do not want to introduce unrelated changes. In this case, it is only a couple of lines so I am not against doing it in this PR. I will leave it up to you.
*. In this case I would argue both cases are very similar but not equivalent as the slice name cannot contain newlines and no free-form text will be introduced there by the user. I like both approaches.
There was a problem hiding this comment.
I opted for consistency. Let me know what you think about it.
internal/setup/yaml.go
Outdated
| if match == nil { | ||
| return nil, fmt.Errorf("invalid slice name %q in %s (start with a-z, len >= 3, only a-z / 0-9 / -)", sliceName, pkgPath) | ||
| } | ||
| if len(yamlSlice.Hint) > 40 || strings.ContainsAny(yamlSlice.Hint, "\n") { |
There was a problem hiding this comment.
Do we want to restrict the input further to only alphanumeric characters and some punctuation?
There was a problem hiding this comment.
As discussed last week I limited the checks done by Chisel to rules that could impact the proper display of the hint (so only the length and the line breaks). Other rules will be enforced in chisel-releases.
There was a problem hiding this comment.
As discussed last week I limited the checks done by Chisel to rules that could impact the proper display of the hint (so only the length and the line breaks).
Yes, I completely agree with that. I was thinking about tabs or other whitespace that we might not be taking into account now as they "occupy" one character but when displayed in the terminal will mess up the output. For example, tabs will probably break our assumptions.
There was a problem hiding this comment.
Good point, I missed that.
I proposed a strict set of characters in the spec. Once we settle on the final set I will rework this check accordingly.
| }, | ||
| relerror: `invalid slice hint for "slice1" in slices/mydir/mypkg.yaml \(len <= 40, no non-standard spaces\): "On\\nmultiple\\nlines.\\n"`, | ||
| }, { | ||
| summary: "Invalid slice hint - non-standard spaces", |
There was a problem hiding this comment.
[Note to reviewers]: This test might be a bit redundant but I wanted to test a non-standard space other than a line break.
| if len(yamlSlice.Hint) > 40 || strings.ContainsFunc(yamlSlice.Hint, func(r rune) bool { | ||
| return !unicode.IsPrint(r) | ||
| }) { | ||
| return nil, fmt.Errorf("invalid slice hint for %q in %s (len <= 40, only printable characters and standard space): %q", sliceName, pkgPath, yamlSlice.Hint) |
There was a problem hiding this comment.
[Note to reviewers]: The way to list the rules here is inconsistent with the other error message for slice names. I tried to express it the same way but it led to something long and hard to read.
Reworking the other error message also led to something longer and harder to understand.
Both are currently expressed as "inclusion sets" but the one for hints is in practice closer to an "exclusion set", thus making it difficult to actually list everything allowed.
So I propose to compromise a bit on consistency to gain in clarity.
2 participants
Support hints on slices. Hints provide optional, concise and
un-opinionated discriminators to help the user select slices.
Hints are defined in slice definition files, in an optional
hintfield under
slices.<slice-name>.In the
chisel findcommand output, theSummarycolumn is replacedby
Hint. As before, the absence of hints is expressed as a dash (“-”).To ensure a proper display, chisel ensures hints are of maximum length
40 characters and do not contain line breaks.
Hints are also visible in the
chisel infocommand output if defined.