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

Move CLI command to /shoutrrr #220

Merged
merged 1 commit into from Jan 25, 2022
Merged

Move CLI command to /shoutrrr #220

merged 1 commit into from Jan 25, 2022

Conversation

arnested
Copy link
Member

@arnested arnested commented Dec 26, 2021

This will make the CLI command installable by running:

go install github.com/containrrr/shoutrrr/shoutrrr@latest

When installing using go install previously the command got installed
with the binary name cli.

@codecov
Copy link

codecov bot commented Dec 26, 2021

Codecov Report

Merging #220 (99a81b9) into main (e46ecc1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #220   +/-   ##
=======================================
  Coverage   76.95%   76.95%           
=======================================
  Files          85       85           
  Lines        2703     2703           
=======================================
  Hits         2080     2080           
  Misses        441      441           
  Partials      182      182           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e46ecc1...99a81b9. Read the comment docs.

@piksel
Copy link
Member

piksel commented Dec 27, 2021

I would probably prefer it to be shoutrrr/shoutrrr instead. I feel like the extra "cmd" doesn't serve any purpose and is less often used as a pattern for CLIs (in my experience).

@simskij
Copy link
Member

simskij commented Dec 27, 2021

I would probably prefer it to be shoutrrr/shoutrrr instead. I feel like the extra "cmd" doesn't serve any purpose and is less often used as a pattern for CLIs (in my experience).

It is a common convention for go applications, however.

@arnested
Copy link
Member Author

I would probably prefer it to be shoutrrr/shoutrrr instead. I feel like the extra "cmd" doesn't serve any purpose and is less often used as a pattern for CLIs (in my experience).

I would also say it's quite common. A few examples:

  • github.com/go-delve/delve/cmd/dlv
  • github.com/fzipp/gocyclo/cmd/gocyclo
  • github.com/golangci/golangci-lint/cmd/golangci-lint
  • golang.org/x/exp/cmd/gorelease
  • golang.org/x/tools/cmd/stringer
  • golang.org/x/tools/cmd/goimports

@piksel
Copy link
Member

piksel commented Dec 28, 2021

Yeah, but those either have multiple binaries (or plan to?), and then it makes sense, but for a single-binary project like this (which instead uses sub-commands), I prefer the style of:

  • github.com/onsi/ginkgo/ginkgo
  • github.com/spf13/cobra/cobra
  • github.com/axw/gocov/gocov
  • github.com/davecgh/go-spew/spew

@simskij
Copy link
Member

simskij commented Dec 28, 2021

You battle it out 😂 I'm down with whichever, although I have a slight preference for the ./cmd pattern.

@arnested
Copy link
Member Author

I'll change it. The most important part for me was just being able to go install with the right binary name. I also have preference for cmd and think it also communicates the intention clearing when browsing the files and directories. But I can live with both.

@arnested
Copy link
Member Author

Change pushed. I'll squash the two commits before this is final and ready to merge.

Putting the command in the folder named ./shoutrrr required small changes to the build script and Dockerfile because the previous build script produced the cli command binary in the root folder under that name.

@piksel
Copy link
Member

piksel commented Dec 29, 2021

Haha, sorry for being unreasonable :D I don't care that much either. I'm just selfishly plotting to reduce the huge amount of characters I have to type :D

@arnested arnested force-pushed the cli-name branch 2 times, most recently from bea80b4 to 7fe1458 Compare December 29, 2021 12:43
This will make the CLI command installable by running:

```
go install github.com/containrrr/shoutrrr/shoutrrr@latest
```

When installing using `go install` previously the command got installed
with the binary name `cli`.
@arnested arnested changed the title Move CLI command to /cmd/shoutrrr Move CLI command to /shoutrrr Dec 29, 2021
@arnested
Copy link
Member Author

I have squashed the commits, and it's ready for merge.

@piksel piksel self-assigned this Jan 2, 2022
@arnested
Copy link
Member Author

@piksel or @simskij, would you merge it?

@simskij simskij merged commit afa4eda into containrrr:main Jan 25, 2022
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.

None yet

3 participants