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

bbin ls prints human readable text #54

Merged
merged 6 commits into from
Apr 8, 2023
Merged

bbin ls prints human readable text #54

merged 6 commits into from
Apr 8, 2023

Conversation

eval
Copy link
Contributor

@eval eval commented Feb 7, 2023

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
Copy link
Contributor

borkdude commented Feb 8, 2023

@eval That looks pretty sweet to me!

@rads What do you think?

@eval eval force-pushed the issue-53 branch 3 times, most recently from 0b676fd to 097f8ca Compare February 8, 2023 11:02
@eval eval changed the title WIP: bbin ls prints human readable text bbin ls prints human readable text Feb 8, 2023
@rads
Copy link
Collaborator

rads commented Feb 22, 2023

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

@eval
Copy link
Contributor Author

eval commented Feb 23, 2023

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

@rads
Copy link
Collaborator

rads commented Mar 4, 2023

@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?

Copy link
Collaborator

@rads rads left a comment

Choose a reason for hiding this comment

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

Need a solution for readable output when small screen widths and long URLs are involved.

@rads
Copy link
Collaborator

rads commented Mar 6, 2023

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
├── [email protected]
├── [email protected]
├── [email protected]

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: [email protected]:~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
Copy link
Collaborator

rads commented Mar 6, 2023

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
Copy link
Contributor

borkdude commented Mar 8, 2023

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
Copy link
Collaborator

rads commented Mar 9, 2023

@borkdude: That sounds good to me.

@eval
Copy link
Contributor Author

eval commented Mar 10, 2023

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
Copy link
Contributor

It sure is pretty

@rads
Copy link
Collaborator

rads commented Mar 22, 2023

@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
Copy link
Contributor Author

eval commented Mar 24, 2023

@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

@eval eval force-pushed the issue-53 branch 3 times, most recently from 04be9c0 to 0a29342 Compare March 25, 2023 15:41
@rads
Copy link
Collaborator

rads commented Apr 3, 2023

@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
Copy link
Collaborator

rads commented Apr 3, 2023

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

@rads rads merged commit 90ac56c into babashka:main Apr 8, 2023
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