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

Introduce new sub command ls #3252

Merged
merged 33 commits into from Dec 18, 2017

Conversation

Projects
None yet
6 participants
@psibi
Copy link
Member

commented Jul 7, 2017

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.

So this patch introduces a new subcommand named ls. Basic usage info:

Usage: stack ls COMMAND [-l|--lts] [-n|--nightly] [--help]
  List latest Stackage snapshots

Available options:
  -l,--lts                 Only show lts snapshots
  -n,--nightly             Only show nightly snapshots
  --help                   Show this help text

Available commands:
  local                    View local snapshot
  remote                   View remote snapshot

Run 'stack --help' for global options that apply to all subcommands.

One can view the remote snapshots like this:

$ stack ls remote
a month ago

Resolver name: lts-8.17
LTS Haskell 8.17 (ghc-8.0.2)

Resolver name: lts-6.35
LTS Haskell 6.35 (ghc-7.10.3)

Resolver name: lts-6.34
LTS Haskell 6.34 (ghc-7.10.3)

Resolver name: nightly-2017-06-02
Stackage Nightly 2017-06-02 (ghc-8.0.2)

Resolver name: nightly-2017-06-01
Stackage Nightly 2017-06-01 (ghc-8.0.2)

Resolver name: lts-8.16
LTS Haskell 8.16 (ghc-8.0.2)

Resolver name: nightly-2017-05-30
Stackage Nightly 2017-05-30 (ghc-8.0.2)

Resolver name: lts-7.24
LTS Haskell 7.24 (ghc-8.0.1)

Resolver name: nightly-2017-05-28
Stackage Nightly 2017-05-28 (ghc-8.0.2)

Resolver name: lts-6.33
LTS Haskell 6.33 (ghc-7.10.3)

Resolver name: nightly-2017-05-27
Stackage Nightly 2017-05-27 (ghc-8.0.2)

Resolver name: nightly-2017-05-26
Stackage Nightly 2017-05-26 (ghc-8.0.2)

Resolver name: nightly-2017-05-25
Stackage Nightly 2017-05-25 (ghc-8.0.2)

Resolver name: nightly-2017-05-24
Stackage Nightly 2017-05-24 (ghc-8.0.2)

Resolver name: nightly-2017-05-23
Stackage Nightly 2017-05-23 (ghc-8.0.2)

Resolver name: nightly-2017-05-22
Stackage Nightly 2017-05-22 (ghc-8.0.2)

Resolver name: lts-7.23
LTS Haskell 7.23 (ghc-8.0.1)

Resolver name: lts-6.32
LTS Haskell 6.32 (ghc-7.10.3)

Resolver name: lts-8.15
LTS Haskell 8.15 (ghc-8.0.2)

Resolver name: nightly-2017-05-20
Stackage Nightly 2017-05-20 (ghc-8.0.2)

Resolver name: nightly-2017-05-19
Stackage Nightly 2017-05-19 (ghc-8.0.2)

Resolver name: nightly-2017-05-18
Stackage Nightly 2017-05-18 (ghc-8.0.2)

Resolver name: nightly-2017-05-17
Stackage Nightly 2017-05-17 (ghc-8.0.2)

Resolver name: lts-8.14
LTS Haskell 8.14 (ghc-8.0.2)

4 weeks ago

Resolver name: nightly-2017-06-12
Stackage Nightly 2017-06-12 (ghc-8.0.2)

Resolver name: nightly-2017-06-11
Stackage Nightly 2017-06-11 (ghc-8.0.2)

Resolver name: lts-8.18
LTS Haskell 8.18 (ghc-8.0.2)

Resolver name: nightly-2017-06-10
Stackage Nightly 2017-06-10 (ghc-8.0.2)

3 weeks ago

Resolver name: nightly-2017-06-19
Stackage Nightly 2017-06-19 (ghc-8.0.2)

Resolver name: nightly-2017-06-18
Stackage Nightly 2017-06-18 (ghc-8.0.2)

Resolver name: lts-8.19
LTS Haskell 8.19 (ghc-8.0.2)

Resolver name: nightly-2017-06-16
Stackage Nightly 2017-06-16 (ghc-8.0.2)

Resolver name: nightly-2017-06-15
Stackage Nightly 2017-06-15 (ghc-8.0.2)

Resolver name: nightly-2017-06-14
Stackage Nightly 2017-06-14 (ghc-8.0.2)

Resolver name: nightly-2017-06-13
Stackage Nightly 2017-06-13 (ghc-8.0.2)

2 weeks ago

Resolver name: nightly-2017-06-23
Stackage Nightly 2017-06-23 (ghc-8.0.2)

Resolver name: lts-8.20
LTS Haskell 8.20 (ghc-8.0.2)

Resolver name: nightly-2017-06-22
Stackage Nightly 2017-06-22 (ghc-8.0.2)

Resolver name: nightly-2017-06-21
Stackage Nightly 2017-06-21 (ghc-8.0.2)

Resolver name: nightly-2017-06-20
Stackage Nightly 2017-06-20 (ghc-8.0.2)

a week ago

Resolver name: nightly-2017-06-30
Stackage Nightly 2017-06-30 (ghc-8.0.2)

Resolver name: lts-8.21
LTS Haskell 8.21 (ghc-8.0.2)

Resolver name: nightly-2017-06-29
Stackage Nightly 2017-06-29 (ghc-8.0.2)

Resolver name: nightly-2017-06-28
Stackage Nightly 2017-06-28 (ghc-8.0.2)

Resolver name: nightly-2017-06-26
Stackage Nightly 2017-06-26 (ghc-8.0.2)

5 days ago

Resolver name: nightly-2017-07-02
Stackage Nightly 2017-07-02 (ghc-8.0.2)

4 days ago

Resolver name: nightly-2017-07-03
Stackage Nightly 2017-07-03 (ghc-8.0.2)

3 days ago

Resolver name: nightly-2017-07-04
Stackage Nightly 2017-07-04 (ghc-8.0.2)

2 days ago

Resolver name: nightly-2017-07-05
Stackage Nightly 2017-07-05 (ghc-8.0.2)

a day ago

Resolver name: nightly-2017-07-06
Stackage Nightly 2017-07-06 (ghc-8.0.2)

If you want to just filter out the lts views, then:

$ stack ls -l remote

Or if you want to just view the nightly ones, then:

$ stack ls -n remote

You can also view your locally stored snapshots:

$ stack ls local

lts-5.10
lts-6.11
lts-6.25
lts-8.12
lts-8.18
lts-8.2
lts-8.4
lts-8.5
lts-8.8
nightly-2017-06-14

You can apply similar filtering as above:

$ stack ls -n local

nightly-2017-06-14

Note that the above PR will work only when the new stackage server code is deployed. Right now all the testing has been done locally.

psibi added some commits Jul 7, 2017

Hlint style fixes
Have to do this to avoid travis CI failures
@harendra-kumar

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2017

@psibi this is awesome! I wanted this from day one. See #1614 which links other issues as well asking for the same thing.

@psibi

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2017

@harendra-kumar Thanks. I didn't know the existence of the other issue. I modeled the interface loosely around nvm. Let me know if you feel there should be any specific changes to the interface.

@harendra-kumar

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2017

I have the following suggestions:

  1. As proposed in #1614 we should make an umbrella list command for different types of things to list. What you have implemented would fall under stack ls snapshots subcommand. You can just create the snapshots subcommand for now and more commands can be added to it later.

  2. The default output snapshots should be the same as or similar to what stack init --solver tries for resolution. It should be concise and useful for common case.

  3. make local and remote as flags of the subcommand snapshots. Even for those limit the output to some threshold or use the pager.

  4. You can use something like --all to produce full output, though it should not be needed if we are using a pager. I guess there is already a pager implementation in stack code base.

ping @mgsloan

@alexanderkjeldaas

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2017

This would be very useful if I could pipe the output through jq, like stack ls remote --format json | jq '.releases[] | sort_by(.date) | select(.name | startswith("lts-")) | .[0]' which would(?) select the latest available lts release.

@decentral1se

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

Note that the above PR will work only when the new stackage server code is deployed.

Seems that change was merged! Nice one.

@psibi, any movement on getting this mergeable? What's left to do?

psibi added some commits Nov 12, 2017

@psibi

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2017

So finally got some time to work on this!

Some updates:

  • Now we have an umbrella ls command as proposed by @harendra-kumar
  • I also use the existing Stack's pager implementation for showing the output
  • local and remote are sub commands of the snapshot command.

This is the current help message:

~/g/stack (stack-ls) $ stack ls snapshots --help
Usage: stack ls snapshots COMMAND [-l|--lts] [-n|--nightly]
  View local snapshot

Available options:
  -l,--lts                 Only show lts snapshots
  -n,--nightly             Only show nightly snapshots
  -h,--help                Show this help text

Available commands:
  remote                   View remote snapshot
  local                    View local snapshot


~/g/stack (stack-ls) $ stack ls snapshots remote --help
Usage: stack ls snapshots remote
  View remote snapshot

Available options:
  -h,--help                Show this help text
@sjakobi

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2017

A few thoughts on the UI/UX:

Personally, I'd prefer a more descriptive name than ls which I find very general. Maybe list-snapshots in analogy to list-dependencies?!

Also, how about simplifying the UI a bit by making the remote subcommand a default, and having --local as a flag? Then stack list-snapshots would list all the remote snapshots.

I guess it would be nice to get some integration for custom/extended snapshots but I haven't used them and have no idea how to achieve that.

@harendra-kumar

This comment has been minimized.

Copy link
Collaborator

commented Nov 20, 2017

@sjakobi the ls command is an umbrella command for listing all kinds of things as discussed in #1614. This is not specific to snapshots. What @psibi has done is two separate things in this PR, 1) introduce the ls command group as per #1614, 2) introduce a subcommand ls snapshot to list the snapshots.

In fact the list-dependencies command should now be made a subcommand of ls. The command group ls would reduce clutter at the top level of CLI where we already have a lot of noise.

I like the idea of making remote as default.

@sjakobi

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2017

@sjakobi the ls command is an umbrella command for listing all kinds of things as discussed in #1614. This is not specific to snapshots. What @psibi has done is two separate things in this PR, 1) introduce the ls command group as per #1614, 2) introduce a subcommand ls snapshot to list the snapshots.

Thanks for clarifying, @harendra-kumar! I should have read the comments here more thoroughly.

[] -> return ()
xs ->
let snaps = BC.concat $ L.map displaySingleSnap xs
in pageByteString snaps

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 27, 2017

Collaborator

There's an issue with always using the pager, which is that someone might want to consume the output programmatically via stdout. Perhaps do isStdoutTerminal <- view terminalL and if true, then run the pager? If false, then assume it's running in a script, and instead send to stdout.

May also be worth considering representing this stuff as Text not ByteString. In a few places above pack is used to go from String to ByteString. I think this is fine, because the Strings will always be ASCII, but it would break on unicode.

This comment has been minimized.

Copy link
@psibi

psibi Nov 27, 2017

Author Member

Thanks, will do this.

@@ -291,6 +292,10 @@ commandLineHandler currentDir progName isInterpreter = complicatedOptions
"Print out handy path information"
pathCmd
Stack.Path.pathParser
addCommand' "ls"

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 27, 2017

Collaborator

I think this should be list-snapshots. There is already some precedent here, with list-dependencies.

How about the default for stack list-snapshots first displaying the remote snapshots, and then displaying the local snapshots. Then, can have stack list-snapshots remote and stack list-snapshots local, particularly useful for when a script wants to access this info.

This comment has been minimized.

Copy link
@psibi

psibi Nov 27, 2017

Author Member

@mgsloan As mentioned by Harendra here, ls is the umberlla command. dependencies will be made to work under this interface ( I can work on this - after the PR get's merged)

How about the default for stack list-snapshots first displaying the remote snapshots, and then displaying the local snapshots. Then, can have stack list-snapshots remote and stack list-snapshots local, particularly useful for when a script wants to access this info.

Display remote snapshots involves an HTTP call and I want to avoid doing that by default. By default, I'm planning to show the local snapshots and then have different options for displaying each of them. Let me know if you think otherwise.

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 27, 2017

Collaborator

Ah, I had not read those comments recently. Ok, I am fine with doing it that way.

@mgsloan

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2017

LGTM for the most part, just a couple comments. Thanks for working on this, it's good stuff! Sorry for letting it languish so long!

In the future it'd be cool to open an issue first so design can be discussed, but no biggie. This way we can hopefully end up with an initial implementation that's closer to what will be merged.

@psibi

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2017

@mgsloan Left some comments on your review.

In the future it'd be cool to open an issue first so design can be discussed, but no biggie.

Yeah, I totally agree. And I guess that's the reason it has been pending for quite some time.

@mgsloan

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2017

And I guess that's the reason it has been pending for quite some time.

Yes, part of it! My initial reaction was "Why does 'stack ls' have such a generic name?" - I should have been more open to a feature PR, but also would be good to discuss first. It became more likely to procrastinate doing review once it was already late. Going to try to avoid that cycle in the future!

I've switched to firefox so hopefully the "my browser window with all the stack issue tabs didn't get ressurrected" problem won't happen anymore.

Actually, I'm not sure if the discuss first approach is always needed, or even whether it is a good thing to require / recommend. After all, necessity is the mother of invention, so if you solve a problem for yourself by adding a feature, it is probably better to do that than to delay on discussion. Hmm, tradeoffs! Maybe a dual approach. Open an issue like "I am working on thing X, it is going to look like this:" and then keep working on it while discussion happens.

psibi added some commits Dec 17, 2017

More changes to ls sub-command
Now the interface is similar to previous commands but we have done
some minor changes:

If a user gives just `stack ls snapshots`, then by default we show the
local snapshots available.

Also based on @mgsloan's input, I don't always use pager now. Also a
new convienence API has been added to `PagerEditor.hs` which exports a
function `pageText`. We use that in our module to pass `Text` content
to pager. (We were using ByteString previously).
@psibi

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2017

Some updates about the previous commit:

The ls subcommand interface is similar to previous one but we have done
some minor changes:

If a user gives just stack ls snapshots, then by default we show the
local snapshots available.

Also based on @mgsloan's input, I don't always use a pager now. Also a
new convenience API has been added to PagerEditor.hs which exports a
function pageText. We use that in our module to pass Text content
to pager. (We were using ByteString previously).

The latest commits addresses all the comments above. So, there is no pending
issues remaining in the PR. (I will fix hlint related style issues if something arises)

@mgsloan

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2017

LGTM, yep just some hlint errors to resolve

psibi added some commits Dec 17, 2017

* 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.
* `pageText` function introduced in `System.Process.PagerEditor`

This comment has been minimized.

Copy link
@mgsloan

mgsloan Dec 18, 2017

Collaborator

Implementation details shouldn't go in the changelog, since stack isn't typically used as a library. All of stack's APIs should be considered to be "internal", in other words they are subject to change even between minor versions.

This comment has been minimized.

Copy link
@psibi

psibi Dec 18, 2017

Author Member

@mgsloan Fixed in the latest push.

@mgsloan

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2017

Good work! Just the one tweak to the changelog and this can be merged.

I like the style of having additive commits while working on a PR, makes reviewing easier. However, once ready for merge, it can be nice to squash it down to a single commit. Not a big deal, I'd merge it either way.

@psibi

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2017

The build failure is happening in Travis because of timeout issue and is not related to the code. I think somebody from the Stack team has to restart the particular failing build.

@harendra-kumar

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2017

However, once ready for merge, it can be nice to squash it down to a single commit.

The main idea here is to not pollute the history with commits that do not add a logically significant change but are just cleanup or random change kind of stuff. So I guess it should be ok to squash it into multiple commits as long as you know that those are logically independent significant steps.

Also, we have added an umbrella ls command here so we should also have a plan (raise an issue?) to move other commands e.g. list-dependencies under it and make sure (keep a watch) that in future other such commands are added under it and not outside.

Nice work @psibi!

@psibi

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2017

@harendra-kumar The github interface allows the committers to squash and merge the commit of this PR. So, I'm leaving the option to the committers. :-) I can squash and merge myself, if others also feel so.

I plan to raise other issues soon.

@decentral1se

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

Nice one @psibi! Should we open a ticket for adding documentation too?

@mgsloan mgsloan merged commit 9711de7 into commercialhaskell:master Dec 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mgsloan

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2017

Thanks for your hard work @psibi ! Other tickets and PRs certainly encouraged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.