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

Make enable/disable nix flag easier to read #8054

Merged
merged 5 commits into from
Mar 24, 2022

Conversation

cbclemmer
Copy link
Collaborator

@cbclemmer cbclemmer commented Mar 20, 2022

This PR is attempting to close #8036

The boolOpt function will not allow for more than one description so I changed the option type from option to multiOption so that the "nix" (--enable-nix and --disable-nix) flags would still be grouped together and allow separate descriptions for disabled and enabled.

As far as the language goes I felt like if the description of "Nix integration" for the --disable-nix was taken out it would make it more clear. It is defined in --enable-nix and --disable-nix is right under it.


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Make sense. Would you add a tiny changelog file and test?

@Mikolaj
Copy link
Member

Mikolaj commented Mar 21, 2022

@jwaldmann: could you review, please?

@jwaldmann
Copy link

I'll look into it, but I'm busy this week.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 21, 2022

Thank you. No rush.

@jneira jneira changed the title Make enble/disable nix flag easier to read Make enable/disable nix flag easier to read Mar 21, 2022
@cbclemmer
Copy link
Collaborator Author

@Mikolaj I have tested the --help flag. I assume that when you run cabal foo --enable-nix you would get some kind of error asking for a "shell.nix" file, but I can't get it to change anything on my pc's version of cabal or on my current source build.
image

@jneira
Copy link
Member

jneira commented Mar 21, 2022

hmm, --enable-nix is a global flag and they should go before any command, so cabal --enable-nix build should work:

> cabal help

Command line interface to the Haskell Cabal infrastructure.

See http://www.haskell.org/cabal/ for more information.

Usage: cabal.exe [GLOBAL FLAGS] [COMMAND [FLAGS]]

.....

Global flags:
.....
 --enable-nix                   Enable Nix integration: run commands through
                                nix-shell if a 'shell.nix' file exists
 --disable-nix                  Disable Nix integration: run commands through
                                nix-shell if a 'shell.nix' file exists
....

@cbclemmer
Copy link
Collaborator Author

@jneira Thanks for the suggestion, I tried that and it doesn't give any more info than it did before (I assume it would throw an error if it didn't find "shell.nix" or "default.nix") but it may be working. I've been trying to go through the command with cabal repl to see if the flag is being set correctly in memory but I'm still new to this repo and it's slow going. Give me a day or so to go through it and write a test.

@jneira
Copy link
Member

jneira commented Mar 22, 2022

well --enable-nix is not applied to v2- commands (and default build is v2-build) so it will be honoured only for cabal --enable-nix v1-build
see #6444 and #4646

@jneira jneira added cabal-install: nix integration re: help-text Concerning option --help and hints given to the user labels Mar 23, 2022
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for adding a test and the changelog

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

You could probably obtain the test quicker by copy-pasting something that depends on an existing harness. but test variety is a boon (except when maintaining tests) and this one is really good.

@cbclemmer
Copy link
Collaborator Author

@Mikolaj Thanks, I wasn't sure where to put the test but it seems like that was good enough. I couldn't find anything that was already in place that tested flags so I just set it up with the functions I could find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: nix integration re: help-text Concerning option --help and hints given to the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--help text seems wrong for --dis/enable-nix
4 participants