-
Notifications
You must be signed in to change notification settings - Fork 697
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
Fix nix config option #8522
Fix nix config option #8522
Conversation
But uh... let me get the tests passing firsts |
It looks like this pr for bytestring is causing the tests to fail. It looks like the test that failed depended upon getting the latest version of bytestring and then building it, it then tried to build the |
@Mikolaj Tests are passing 🎉 |
@cbclemmer: thank you for the diagnosis of the spurious CI failure. Indeed that seems to have been the problem and we have since fixed it after a lot of confusion. A pity I haven't looked at this PR earlier. :) @mhwombat, might we solicit your review? Even just an absent-minded look at the code, a checkout, a run and a report in a comment that it works fine now? I will have a closer look now, but another pair of eyes is very welcome (and also we formally require two reviews for each PR). |
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.
Thanks a lot for the fix, the test and the changelog.
optArg' "(True or False)" maybeStringToBoolFlag (\case | ||
Flag True -> [Just "enable"] | ||
Flag False -> [Just "disable"] | ||
NoFlag -> [Just "disable"]) "" ["nix"] | ||
"Nix integration: run commands through nix-shell if a 'shell.nix' file exists (default is False)", |
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.
Could this trivially be expressed using trueArg
or boolOpt
or similar? If not trivially, the extra maybeStringToBoolFlag
function is fine.
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.
@Mikolaj That part is to make the nix: True|False
part work in the config file. The issue was that only enable-nix
and disable-nix
worked for both the command line arg and the config file. This makes it so that all three work while showing the correct help text. I tried a few different things and none of them satisfied all the things I wanted to do.
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.
Makes sense. I'm just asking about a technicality: optArg'
is rather low level, while trueArg
or boolOpt
may implement some or all of the functionality in maybeStringToBoolFlag
, so perhaps they are the better functions to use here?
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 can't remember exactly why I didn't go with it, but I'll try again and let you know
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.
@Mikolaj looked at it again and this is what I found:
boolOpt
creates two separate help text fields with the only difference being that "enable" or "disable" is prepended to the help text. This is what the original pr was addressing because that was what was being used there and creating bad help text.
trueArg|falseArg
I can use this in conjunction with boolOpt
to get the help text to work but they end up overriding each other in practice and the tests fail.
optArg
is actually a more verbose version of optArg'
so it saves characters and complexity to use optArg'
over optArg
(I assume this is why optArg'
is used so much more in general)
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.
Thank you for the analysis. TIL. Once a second review arrives, let's ship it!
The CI failure is probably caused by https://bugs.launchpad.net/ubuntu/+source/git/+bug/1993586 (and by us using the latest Ubuntu container). |
May be fixed in #8546 (if CI passes there). |
The CI fix seems to work. Let me rebase on it... |
@mergify rebase |
✅ Branch has been successfully rebased |
@Mikolaj Thank you, I was really confused |
This time it seems it failed due to some corruption, hanging and cancellation after 360min. I'm restarting. |
Succes. Quick, @cbclemmer, set one of the merge_me labels to merge the PR while it still passes CI. :) |
Edit: with mergify waiting 2 days after the label is set, as per the process. |
* Fix nix config option * Add changelog file * Fix whitespace * Reduce string to bool flag fn Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Closes #8452
I did not create a test for the config files when I made #8054. This should test config files for leaving out nix, "nix: True", and "nix: False".
This also adds a line in the
--help
output for the nix option, it may not be necessary and can be easily removed if it is not needed.@mhwombat it would be useful for you to confirm that this fix removes the error you were seeing
@Mikolaj I assigned you to review, if this isn't correct, please redirect the review.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!