Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add confirm prompt #1834
Add confirm prompt #1834
Changes from 19 commits
ec0794c
8b28f7b
3c266d8
a3fd212
4a11619
d28ac60
fe22811
04b6b7d
d03a45a
bc7636e
233b177
12e63a6
3c90d6a
d1c2ed7
05faf55
dec8ed7
2734e93
6545e54
145bc89
bc1f5f1
4c1bf1c
7589a2b
b86ded7
5987265
fcea9a6
2ee15f0
b528c0d
d6caa19
50d47c7
e8387bc
41c8071
76ca7bb
bc82faf
b8b3317
cb43e0f
1c7585c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Actually, we only have one attribute that accepts a string literal, and if we're accepting multiple arguments, we would want to accept commas between them, so we can make
arguments
be anOption
.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.
Comma-separated arguments are now accepted, do should comma+space also be accepted?
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.
Ah, no sorry, what I was getting at is that we don't have any multi-argument attributes, so we shouldn't bother either with parsing multiple arguments, with or without commas. We can just parse
( 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.
Alright, but now it is implemented, do you want it removed? It's like 2 lines and all the scaffolding is there now if there's a need/want for features that take advantage of recipe attributes with arguments. Off the top of my head it could be stuff like
[linux("arm64", "amd64")]
that enables recipes based on OS + Architecture, or adding kernel version for requirements etc.But I respect if you invoke YAGNI, then I'll revert it.
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 we'll probably add it later, but that could be a while, so I'd assume just remove it. It also makes
Attribute::with_arguments
simpler, since it can take anOption
instead of aVec
.