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.

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

Output current default priority in dpkg-reconfigure completion #3522

Conversation

MagicMuscleMan
Copy link
Contributor

I wanted to separate this commit into another pull request, as I am not sure if dynamically changing descriptions are okay (but I included the former change, which is already merged, as it logically builds upon it).

This might be a bit over the top, but getting the information that a default priority threshold is used without knowing what that value is or how to find out might not be so useful after all. Thus, change the completion to include this information dynamically.

The dpkg-reconfigure command is used on Debian and Ubuntu based systems to reconfigure packages.

According to the relevant manpage's the commited completion file should be complete.
This might be a bit over the top, but getting the information that a default priority threshold is used without knowing what that value is or how to find out might not be so useful after all. Thus, change the completion to include this information dynamically.
@@ -8,7 +8,7 @@ complete -x -f -c dpkg-reconfigure -s h -l help --description 'Display help'
# General options
complete -f -c dpkg-reconfigure -s f -l frontend -r -a "dialog readline noninteractive gnome kde editor web" --description 'Set configuration frontend'
complete -f -c dpkg-reconfigure -s p -l priority -r -a "low medium high critical" --description 'Set priority threshold'
complete -f -c dpkg-reconfigure -l default-priority --description 'Use default priority threshold'
complete -f -c dpkg-reconfigure -l default-priority --description "Use current default ("(echo get debconf/priority | debconf-communicate | string match -r '\w*$')") priority threshold"
Copy link
Member

Choose a reason for hiding this comment

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

Is this string match supposed to remove leading whitespace? If so, string trim would be a bit more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

debconf-communicate returns something like 0 medium or 10 high, where the first token seems to be a return (failure) code and the second token is what we are looking for. The string match selects the second token, thus string trim won't work. I didn't find a command line option to avoid returning the first token.

Splitting and indexing would be the most logical thing in my opinion, but I wanted to avoid introducing state (a variable) and something like
echo "0 medium" | string split --index 2 " "
does not exist.

However I just learned, that Fish can index subexpressions, which is even more flexible, i.e.
echo (echo "0 medium" | string split " ")[2]
works, although it looks a bit complex in this example.

But as a subexpression is already used, this actually gets easier
complete -f -c dpkg-reconfigure -l default-priority --description "Use current default ("(echo get debconf/priority | debconf-communicate | string split ' ')[2]") priority threshold".

Should I create a new branch with that change?

P. S. Sorry for the need to cherry-pick. I haven't thought it through.

Copy link
Member

Choose a reason for hiding this comment

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

echo (echo "0 medium" | string split " ")[2]
works, although it looks a bit complex in this example.

That'll only work if it only ever prints one thing.

The current expression is almost fine for this, except it should not print blank lines probably - try \w+$. (Yes, this still relies on the fact that the matching is greedy i.e. string match -r always returns the longest possible match, but that's the more expected behavior anyway)

This way, it'll select the last token.

Copy link
Member

Choose a reason for hiding this comment

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

And no need to put it onto another branch/pr. Just continue from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll only work if it only ever prints one thing.

I assume that the output always consists of two tokens, but I agree that \w+$ is better than \w*$. Indexing with [-1] would be the same as matching \w+$, wouldn't it? I like the indexing a bit more, but I'd go with that regex if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I like the indexing a bit more, but I'd go with that regex if you prefer.

The problem with the indexing is also that we print an ugly error for an index > 1 (or < -1) if it doesn't exist - echo $a[2]. So the "[2]" is unworkable, currently.

For that reason using indexes on command substitutions is not something I get in the habit of doing - I wouldn't call it "idiomatic fish". This is a minor style nit, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I changed the regular expression and commited it to a new branch MagicMuscleMan:default-priority-dpkg-reconfigure containing only this commit. I also added the change to the branch for the current pull request. So you can choose which commit you want to use.

@faho
Copy link
Member

faho commented Nov 4, 2016

It's nice if a pr-branch contains just the necessary commits. In this case I'll have to cherry-pick. Not the end of the world, but something to keep in mind for next time.

As suggested by faho the regexp should not match an empty string.
@faho faho added this to the fish 2.5.0 milestone Nov 26, 2016
@faho
Copy link
Member

faho commented Nov 26, 2016

Merged as ded6e72. Thanks!

@faho faho closed this Nov 26, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants