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

Port list-dependencies under the new ls sub command #3706

Merged
merged 7 commits into from
Jan 4, 2018

Conversation

psibi
Copy link
Member

@psibi psibi commented Dec 25, 2017

This PR also deprecates the existing interface. Fixes #3669

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

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

Copy link
Member

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Looks good! I wonder if we could add some deprecation message like over here.

It isn't done much, but would be nice.

ChangeLog.md Outdated
@@ -13,6 +13,8 @@ Other enhancements:
* A new sub command `ls` has been introduced to stack to view
local and remote snapshots present in the system. Use `stack ls
snapshots --help` to get more details about it.
* `list-dependencies` has been deprecated. The functionality has
to accessed through the new `ls dependencies` interface.
Copy link
Member

Choose a reason for hiding this comment

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

to->to be

Copy link
Member

Choose a reason for hiding this comment

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

We should also link out to the relevant issue here?

src/main/Main.hs Outdated
@@ -385,7 +385,7 @@ commandLineHandler currentDir progName isInterpreter = complicatedOptions
cleanCmd
cleanOptsParser
addCommand' "list-dependencies"
"List the dependencies"
"DEPRECATED: Use ls dependencies instead. Will be removed in next major version."
Copy link
Member

Choose a reason for hiding this comment

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

While it's a safe bet, no other deprecation notice specifies a time :/

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually haven't been so good at removing deprecated stuff.

Copy link
Contributor

@mgsloan mgsloan Dec 27, 2017

Choose a reason for hiding this comment

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

I'd say we should have list-dependencies output a warning (to stderr) that it may be removed at some point. Thing is, people do use it in scripts. We should probably wait until stack 2.0 or 3.0 to remove it. Best to avoid needless script breakage. Could hide it from the --help output next version, though.

@mgsloan
Copy link
Contributor

mgsloan commented Dec 27, 2017

LGTM, I'd like it to warn for the old list-dependencies command (but to stderr not stdout, since scripts use its stdout)

@psibi
Copy link
Member Author

psibi commented Jan 1, 2018

Pushed some commits that fixes the above concerns.

@mgsloan
Copy link
Contributor

mgsloan commented Jan 4, 2018

LGTM, thanks! 👍

@mgsloan mgsloan merged commit ca9ceb1 into commercialhaskell:master Jan 4, 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