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 NixStyleOptions #6745

Merged
merged 1 commit into from
May 4, 2020
Merged

Make NixStyleOptions #6745

merged 1 commit into from
May 4, 2020

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Apr 30, 2020

@m-renaud
Copy link
Collaborator

Oh geeze, I'm pretty sure in the previous PR we were talking past each other about refactorings and completely different levels of the control flow 😝 I wasn't trying to make any changes to the "command" options/registration/invocation themselves, I was cleaning up the internal implementation details so that we could separate out the lib-only logic (see #6746 for a start).

@phadej phadej requested a review from m-renaud May 1, 2020 07:04
@m-renaud
Copy link
Collaborator

m-renaud commented May 1, 2020

Overall LGTM!

High level comment on naming: the use of "NixStyle" in the name of the options seems overfitted to the original motivation for the v2- style commands. There's nothing "nix" specific about these commands, these are just the options for the v2/new style commends which are now the default, if anything I would name these just simply CommandOptions and CommandFlags and any types related to the old-style commands (if they overlap/conflict) could be renamed to something like LegacyOptions / LegacyFlags. As a concrete example, CmdHaddock now references something with Nix in the name that is unrelated to Nix integration, which I can easily see leading to confusion.

I think this is inline with your #6105 as well, but reduces confusion for anyone looking at the codebase as opposed to the docs.

@phadej
Copy link
Collaborator Author

phadej commented May 2, 2020

The #6150 is undecided, yet my impression is that using nix-style local builds is what has most consensus on.

I'd avoid Legacy or any kind of old/new - relative i.e. non absolute - names. We have already Legacy and project config, and it's not related to v1 stuff AFAICT.

For that reason I don't like CommandFlags, as we still have v1- stuff pretty strongly in the codebase. We disagree on documentation already, but in code I'd strongly prefer to rather be explicit than rely on implicit "whatever is default now".

@m-renaud
Copy link
Collaborator

m-renaud commented May 2, 2020

Eh fair enough, I'm a -1 personally on "Nix style" but I'll leave my 2c on the other thread. This change is a big improvement over the tuple manipulation that was there before so lets leave any name bikeshedding until afterwards.

@phadej phadej merged commit 68320f1 into master May 4, 2020
@phadej phadej deleted the install-commandui branch May 4, 2020 09:40
@phadej phadej mentioned this pull request Jul 10, 2020
@phadej phadej added this to the 3.4.0.0-rc1 milestone Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants