babashka / bbin

Install any Babashka script or project with one command
MIT License
139 stars 9 forks source link

`bbin ls` prints human readable text #54

Closed eval closed 1 year ago

eval commented 1 year ago

Please answer the following questions and leave the below in as part of your PR.

Example:

Screenshot 2023-02-07 at 18 48 20

Feedback welcome.

borkdude commented 1 year ago

@eval That looks pretty sweet to me!

@rads What do you think?

rads commented 1 year ago

Thanks for the PR. I'll take a look at this soon.

eval commented 1 year ago

Amended a fix for the lint-warnings (that failed the build).

rads commented 1 year ago

@eval @borkdude: It looks like we have some challenges with smaller screen widths. Here's what the new version looks like on my MacBook Air.

We can try to fix this problem within bbin, however before we go too far down the rabbit hole, would this code make more sense as a standalone library to convert EDN to CLI tables? If we were using nbb I'd pull in cli-table3... maybe it would be useful to have a similar lib for bb?

rads commented 1 year ago

After thinking about this a bit more, I wonder if we should replace the table with something simpler and more compact. For example, here's the output of npm ls:

$ npm --location=global ls
/Users/rads/.nvm/versions/node/v16.16.0/lib
├── npm@8.11.0
├── sql-formatter@12.1.2
├── squint-cljs@0.0.10

We could do something similar (the * represents bold text):

*hello* (sha: 594ffad)
*neil* (tag: v0.1.56)
*bbin-dev* (url: file:///Users/rads/src/bbin)

Or a more extended version:

*hello (ht.sr.rads/watch-private)*
URL: git@git.sr.ht:~rads/watch-private
SHA: 594ffadceea43313789f85a42a120791ce35e323

*neil (io.github.babashka/neil)*
URL: https://github.com/babashka/neil.git
Tag: v0.1.56
SHA: abea182499897eda40088d24b647e66099ef94ed

*bbin-dev*
URL: file:///Users/rads/src/bbin

What do you think?

rads commented 1 year ago

Maybe we can have two commands like npm does: bbin ls for the compact output, bbin info for extended output on a particular script.

borkdude commented 1 year ago

Maybe it would make sense to print like neil: in general the output if neil can be piped back into neil subcommands.

$ neil dep search json
:lib cheshire/cheshire :version "5.11.0" :description "JSON and JSON SMILE encoding, fast."
:lib tigris/tigris :version "0.1.2" :description "Stream-to-stream JSON string encoding"
:lib json-html/json-html :version "0.4.7" :description "Provide JSON and get a DOM node with a human representation of that JSON"
:lib ring/ring-json :version "0.5.1" :description "Ring middleware for handling JSON"

E.g.:

bbin ls

:name "foo" :location ... 
:name "bar" :location ...
rads commented 1 year ago

@borkdude: That sounds good to me.

eval commented 1 year ago

My vote goes to 'team human(-readable)', and make this listing easy on the eyes.

So alternatively, using this PR, we'll squash columns bbin/url, git/url and local/root into one column location, and columns git/sha and git/tag into version:

Screenshot 2023-03-10 at 15 48 52
borkdude commented 1 year ago

It sure is pretty

rads commented 1 year ago

@eval: When I split my terminal on my MacBook Air with my current font size, I get 71 characters horizontally. If we can get it to work without wrapping in 71 characters then I'm open to merging this PR in. This will require truncation since URLs are often much longer than 71 characters plus each column needs its own space.

eval commented 1 year ago

@rads thanks for reconsidering this PR.

I added a truncate-fn that truncates urls from the middle, e.g. "example.org/path/to/release/v1.2.3/server.jar" becomes "example.org/path/…v1.2.3/server.jar" to prevent multiple urls looking alike in a narrow table. It also strips file:// and https:// from the location if the tables exceeds the screen-width. It acts like this (the last frame shows a terminal with 62 columns): bbin-ls

rads commented 1 year ago

@eval: I appreciate your patience so far. I'm planning to do a final review this week. I'll put the new functionality behind a flag until I release 0.2.0 since it's technically a breaking change. This will coincide with #35 which is another breaking change (and my next priority on the list of issues).

rads commented 1 year ago

Woops, I meant to link to #35 instead of #34.