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

Use default flags only for new-* path #4657

Merged
merged 1 commit into from Aug 2, 2017

Conversation

alexbiehl
Copy link
Member

Fixes #4581.
With 1e90ae4 flags from the global
configuration would be ignored. This patch makes sure the defaults are
mixed in only for the new-build code path.

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.

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

@mention-bot
Copy link

@alexbiehl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @23Skidoo, @ezyang and @dcoutts to be potential reviewers.

@alexbiehl alexbiehl requested a review from hvr August 2, 2017 12:14
@23Skidoo
Copy link
Member

23Skidoo commented Aug 2, 2017

Ah yeah, that's because command line flags are mappended on top of the ones loaded from the config, so when they're non-mempty from the beginning, they always override whatever was in the config. I think we should just revert 1e90ae4.

]

type Action = GlobalFlags -> IO ()

defaultsCmdWrapper :: ((ConfigFlags, ConfigExFlags, InstallFlags, HaddockFlags) -> [String] -> GlobalFlags -> IO ())
Copy link
Member

@23Skidoo 23Skidoo Aug 2, 2017

Choose a reason for hiding this comment

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

I'd be more happy if instead of doing this we applied the defaults directly inside Cmd*.*Action. That'd be a tad more verbose, but also more explicit.

So e.g. for CmdBuild.buildAction you'd have:

buildAction cliFlags@(_configFlags, _configExFlags, _installFlags, _haddockFlags) ... =
let (configFlags, configExFlags, installFlags, haddockFlags) = applyFlagDefaults cliFlags
...

Copy link
Member

@hvr hvr Aug 2, 2017

Choose a reason for hiding this comment

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

@23Skidoo we could use ViewPatterns, then it would look like

buildAction (applyFlagDefaults -> (configFlags, configExFlags, installFlags, haddockFlags))

EDIT: syntax typo

@hvr
Copy link
Member

hvr commented Aug 2, 2017

@23Skidoo if we only/just revert 1e90ae4 then new-build regresses again by running into errors whenever fromFlag is called on some NoFlag. There was a reason for 1e90ae4...

@hvr
Copy link
Member

hvr commented Aug 2, 2017

@23Skidoo it's all green! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants