Skip to content

outdated: accept --project-file #5474

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

Merged
merged 4 commits into from
Jul 31, 2018

Conversation

quasicomputational
Copy link
Contributor

This doesn't get anywhere near the improvements suggested in #4831,
but it's a very respectable improvement over the status-quo for not
much effort.

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.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

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

New test added for this.

This doesn't get anywhere near the improvements suggested in haskell#4831,
but it's a very respectable improvement over the status-quo for not
much effort.
@hvr hvr requested a review from 23Skidoo July 30, 2018 16:36
@@ -1336,6 +1336,10 @@ The following flags are supported by the ``outdated`` command:
``--new-freeze-file``
Read dependency version bounds from the new-style freeze file
(``cabal.project.freeze``) instead of the package description file.
``--project-file`` *PROJECTFILE*
Copy link
Member

@hvr hvr Jul 30, 2018

Choose a reason for hiding this comment

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

How does this interact w/ the --new-freeze-file flag?

and more importantly, what happens if you say

cabal --project-file=foo outdated --project-file=bar --new-freeze-file

or

cabal --project-file=foo outdated --new-freeze-file

?

(i.e. we already have a global --project-file= flag... and it appears you've added a 2nd command-local one?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last one wins (or it should be). I'll add a test and make sure it's explicitly mentioned in the docs.

outdatedProjectFile (\_ flags -> flags { outdatedProjectFile = pure "cabal.project" })
(noArg mempty)

,option [] ["project-file"]
Copy link
Member

Choose a reason for hiding this comment

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

Something doesn't smell right to me about duplicating the global --project-file= flag as a local outdated sub-command flag. What does this make possible that wasn't possible before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I don't understand this question. Where is the global --project-file flag being defined? outdated doesn't currently accept it:

$ cabal outdated --project-file=foo.project
cabal: unrecognized 'outdated' option `--project-file=foo.project'
$ cabal --version
cabal-install version 2.2.0.0
compiled using version 2.2.0.1 of the Cabal library 

Bear in mind that outdated isn't really a new-style command: it's more like a v1 command that happens to know a little bit about new-style freeze files. new-outdated as outlined in #4831 might be a worthy project, but that's a much bigger task. I think that this patch has a good power-to-weight ratio.

Copy link
Member

Choose a reason for hiding this comment

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

Oh you're right, I stand corrected. I confused --project-file= to be a global one (like --store-dir=)

Now this is starting to make more sense. But if we're going to migrate to make the new-*-style commands default, then it's also kinda strange to have --project-file= imply a side-effect; rather than have it act like merely setting a property, and requiring an explicit --new-freeze-file; that would feel more consistent to me w/ how --project-file= usually acts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone ahead and implemented that. In the future we'll probably want to remove --new-freeze-file outright; I've made sure that --project-file without it errors out, so that we've got freedom to make that change.

Now the behaviour doesn't switch on just --project-file's
presence.
Also improve the wording slightly about the `--new-freeze-file`
requirement.

[skip ci]
@hvr hvr added this to the 2.4 milestone Jul 31, 2018
@23Skidoo
Copy link
Member

Bikeshedding: should --new-freeze-file be called --v2-freeze-file in light of #5429?

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM.

@23Skidoo 23Skidoo merged commit 402ebb8 into haskell:master Jul 31, 2018
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.

3 participants