feat: hints on slices by upils · Pull Request #263 · canonical/chisel

upils · 2026-01-12T08:46:15Z

Have you signed the CLA?

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 hint
field under slices.<slice-name>.
In the chisel find command output, the Summary column is replaced
by 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 info command output if defined.

github-actions · 2026-01-12T08:51:27Z

Command	Mean [s]	Min [s]	Max [s]	Relative
`BASE`	12.198 ± 0.094	12.053	12.383	1.02 ± 0.01
`HEAD`	11.999 ± 0.073	11.909	12.150	1.00

letFunny

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.

letFunny · 2026-01-12T08:57:03Z

+						/usr/bin/cc:
+		`,
+	},
+	relerror: `invalid slice hint for "slice1" in slices/mydir/mypkg.yaml: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" \(len <= 40, no line breaks\)`,


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.

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?

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.

I opted for consistency. Let me know what you think about it.

letFunny · 2026-01-12T08:58:20Z

 		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") {


Do we want to restrict the input further to only alphanumeric characters and some punctuation?

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.

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.

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.

upils · 2026-01-13T10:19:45Z

+	},
+	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",


[Note to reviewers]: This test might be a bit redundant but I wanted to test a non-standard space other than a line break.

upils · 2026-01-13T13:51:53Z

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


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

There is a balance here to strike. We want to be precise but we do not want to create an overly complex error message that will confuse the user. If you look at what we do for slice names, the message is not 100% coherent with the regex but it is enough for the user to know how to write a good slice name.

We can do a similar thing here, I think "printable characters" is too technical as it refers to a concrete subset of unicode. Also, my assumption is that no user is going to use non-printable characters accidentally. So I think we can simplify the message to what you had before, (len <= 40, only " " allowed).

only " " allowed

This, alone, looks misleading to me. We are not only allowing spaces. What about " " only space allowed or only standard space allowed?

Discussed offline. I settled on only letters, numbers, symbols and " ".

letFunny

Approving tentatively after my two minor comments are addressed. This looks simple and pragmatic, I am glad we could simplify the requirements to land something that fits in Chisel and has very few moving parts.

letFunny · 2026-01-14T09:48:51Z

+		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 is a balance here to strike. We want to be precise but we do not want to create an overly complex error message that will confuse the user. If you look at what we do for slice names, the message is not 100% coherent with the regex but it is enough for the user to know how to write a good slice name.

We can do a similar thing here, I think "printable characters" is too technical as it refers to a concrete subset of unicode. Also, my assumption is that no user is going to use non-printable characters accidentally. So I think we can simplify the message to what you had before, (len <= 40, only " " allowed).

niemeyer

This looks good. Only one issue with the error messages.

niemeyer · 2026-01-19T16:50:19Z

 		match := apacheutil.SnameExp.FindStringSubmatch(sliceName)
 		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)
+			return nil, fmt.Errorf("invalid slice name in %s (start with a-z, len >= 3, only a-z / 0-9 / -): %q", pkgPath, sliceName)


We don't want the long list of things between the meaningful information and the error saying what the issue was. Maybe this instead:

"invalid slice name in %s: %q (must start with ...)"

This way we keep the name last, which is what I think you want here.

Reworked and harmonized. I applied your suggestion on this message and harmonized the new one with following ones, relying on the display of the slice name.

niemeyer · 2026-01-19T16:56:32Z

+			return !unicode.IsPrint(r)
+		})
+		if len(yamlSlice.Hint) > 40 || hintNotPrintable {
+			return nil, fmt.Errorf("invalid slice hint for %q in %s (len <= 40, only letters, numbers, symbols and \" \"): %q", sliceName, pkgPath, yamlSlice.Hint)


Same issue here.

See answer here #263 (comment)

Harmonize the hint error with other **following** errors since at this step the slice name is known and we do not need to display the full package path.

niemeyer

Thanks, let's give this a shot

niemeyer · 2026-01-26T16:20:34Z

+			return !unicode.IsPrint(r)
+		})
+		if len(yamlSlice.Hint) > 40 || hintNotPrintable {
+			return nil, fmt.Errorf("slice %s has invalid hint %q (must be len <= 40, only contain letters, numbers, symbols and \" \")", SliceKey{pkgName, sliceName}, yamlSlice.Hint)


spaces is a better way to say it than " ", in this context, but we can fix that later

upils added 3 commits January 9, 2026 17:36

feat: support hints

91126a2

test: hint marshalling

8e16979

refactor: apply PR suggestion

5a14e13

letFunny reviewed Jan 12, 2026

View reviewed changes

upils added 2 commits January 12, 2026 11:10

style: apply PR suggestions

35173f8

style: improve test hint values

92ffeb7

upils requested a review from letFunny January 12, 2026 10:16

upils added 2 commits January 13, 2026 09:03

fix: forbid all non-standad spaces from hints

3b29f36

fix: display consistent validation errors

aec7d72

upils commented Jan 13, 2026

View reviewed changes

fix: only allow printable chars and space in hints

93131d9

upils commented Jan 13, 2026

View reviewed changes

letFunny approved these changes Jan 14, 2026

View reviewed changes

upils added 2 commits January 14, 2026 11:45

refactor: apply PR suggestions

56d7085

style: rework invalid hint error

8ec40a9

letFunny added the Simple Nice for a quick look on a minute or two label Jan 15, 2026

upils mentioned this pull request Jan 19, 2026

docs: add reference for hints canonical/chisel-docs#54

Merged

niemeyer requested changes Jan 19, 2026

View reviewed changes

fix: rework name and hint error message

c0d780c

upils requested a review from letFunny January 20, 2026 13:22

upils added 3 commits January 22, 2026 15:14

refactor: harmonize errors

381eeaf

Harmonize the hint error with other **following** errors since at this step the slice name is known and we do not need to display the full package path.

ci: rerun

046ff8c

refactor: avoid partial init

3db4002

letFunny approved these changes Jan 22, 2026

View reviewed changes

upils added 3 commits January 23, 2026 08:51

ci: rerun

770c121

ci: rerun

2cbc779

ci: rerun

ec294ce

niemeyer approved these changes Jan 26, 2026

View reviewed changes

niemeyer merged commit 41591a8 into canonical:main Jan 26, 2026
18 checks passed

cjdcordeiro mentioned this pull request Feb 10, 2026

ci: add hint validation checks canonical/chisel-releases#898

Merged

3 tasks done

Conversation

upils commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

upils Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

upils Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

upils commented Jan 12, 2026 •

edited

Loading

github-actions Bot commented Jan 12, 2026 •

edited

Loading

upils Jan 20, 2026 •

edited

Loading

upils Jan 20, 2026 •

edited

Loading