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

Add file IO into CLI #6

Merged
merged 2 commits into from
Feb 12, 2021
Merged

Conversation

diamondburned
Copy link
Contributor

@diamondburned diamondburned commented Feb 11, 2021

This pull request adds the flag -o to set the output; it also accepts the first argument for the input. Both IO arguments can take - for stdio.

Usage:

―❤―▶ echo "bruh moment" | go run ./cmd/bottom/ | go run ./cmd/bottom -d 
bruh moment

This pull request also moved the CLI into ./cmd/bottom for idiomaticity. The command to install it would therefore be:

go get github.com/bottom-software-foundation/bottom-go/cmd/bottom

API breakage:

This pull request breaks the existing EncodeTo function signature to remove the total bytes written. This is because the function will either write everything successfully or error out, and the caller can manually keep track of the written length by wrapping the writer.


This PR closes #2.

@K1ngjulien
Copy link
Contributor

cool! this doesn't support passing in an input string directly, correct? (i.e bottom "bruh moment" )

@diamondburned
Copy link
Contributor Author

cool! this doesn't support passing in an input string directly, correct? (i.e bottom "bruh moment" )

It doesn't, so you'll have to use ./bottom <<< "string". The first argument is used for the input file.

@nihaals
Copy link
Member

nihaals commented Feb 11, 2021

I think we should Go for something like #2 (comment) as it feels more familiar and is much nicer when not wanting to encode a file

@diamondburned
Copy link
Contributor Author

diamondburned commented Feb 11, 2021

I think we should Go for something like #2 (comment) as it feels more familiar and is much nicer when not wanting to encode a file

I'm not sure what's more familiar about that. Almost any Unix tools I know of does something similar:

―❤―▶ base64 --help
Usage: base64 [OPTION]... [FILE]
Base64 encode or decode FILE, or standard input, to standard output.

With no FILE, or when FILE is -, read standard input.

Mandatory arguments to long options are mandatory for short options too.
  -d, --decode          decode data

―❤―▶ sha256sum --help
Usage: sha256sum [OPTION]... [FILE]...

―❤―▶ cat --help
Usage: cat [OPTION]... [FILE]...

Everybody who's remotely familiar with the terminal knows the echo text | command construct to feed into stdin.

@nihaals
Copy link
Member

nihaals commented Feb 12, 2021

GNU's base64 was actually what I originally based the design on despite other implementations using different syntax so I agree it makes sense.

I've seen a mix, most of the "standard" tools use files but there's still popular tools that either support both or you prefix files with a flag (ffmpeg comes to mind).

I don't have a strong preference but based on feedback I'm absolutely willing to reconsider. If we stick to base64, which after you bringing it up has made me change my mind towards using only files, then only allowing files is the way to go and will probably make it easier to understand for new users (we can also specify this inspiration in the README). A potential argument is that the majority of uses of bottom is for a quick encode/decode, so requiring a file might just make it more annoying.

@K1ngjulien what do you think?

@K1ngjulien
Copy link
Contributor

K1ngjulien commented Feb 12, 2021

I'm not sure what's more familiar about that. Almost any Unix tools I know of does something similar

Yeah you're right, I hadn't really thought of that. The standard unix way is definitely a more familiar version and the way to go.

Tho I agree that most people will be using bottom in a one off fashon for quck encodes. So maybe an additional flag for passing in strings directly could be added, which would error when also passing a filename at the same time:

$ bottom -t "input text" 
<bottom text>
$ bottom input.txt -t "hello"
Error

This way you wouldn't have to pipe in all your text from echo

Thougts?

@diamondburned
Copy link
Contributor Author

I'm not sure what's more familiar about that. Almost any Unix tools I know of does something similar

Yeah you're right, I hadn't really thought of that. The standard unix way is definitely a more familiar version and the way to go.

Tho I agree that most people will be using bottom in a one off fashon for quck encodes. So maybe an additional flag for passing in strings directly could be added, which would error when also passing a filename at the same time:

$ bottom -t "input text" 
<bottom text>
$ bottom input.txt -t "hello"
Error

This way you wouldn't have to pipe in all your text from echo

Thougts?

Is doing echo "text" | bottom or bottom <<< "text" that much work?

@K1ngjulien
Copy link
Contributor

TIL about command <<< "string" 😅😅

Well in that case, ship it🚀

@nihaals nihaals merged commit 2950ed5 into bottom-software-foundation:main Feb 12, 2021
@nihaals
Copy link
Member

nihaals commented Feb 13, 2021

Related #7

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.

Support files and stdin
3 participants