-
Notifications
You must be signed in to change notification settings - Fork 128
Add --version option with full credits #34 #36
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome first attack, thanks a lot @brooksryba for submmitting this PR, and so quickly! Sorry I couldn't come back to you faster; we're having some state holidays this weekend here in Poland, and they made me more busy with life stuff than I expected to be... Please take care to review the tweaks I suggest that you do on this change. There's still some work needed before we can have this close the #34.
Thanks again! ❤️
@@ -75,13 +75,32 @@ var ( | |||
unsafeMode = pflag.Bool("unsafe-full-throttle", false, "enable mode in which command is executed immediately after any change") | |||
outputScript = pflag.StringP("output-script", "o", "", "save the command to specified `file` if Ctrl-X is pressed (default: up<N>.sh)") | |||
debugMode = pflag.Bool("debug", false, "debug mode") | |||
versionFlag = pflag.Bool("version", false, "display version and credits") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to run go fmt
on the code
os.Stdout.WriteString("up: \n") | ||
os.Stdout.WriteString("up: \tpflag\n") | ||
os.Stdout.WriteString("up: \t\thttps://github.com/spf13/pflag\n") | ||
os.Stdout.WriteString("up: \t\tLicense: BSD-3-Clause, BSD-Source-Code, BSD-3-Clause-No-Nuclear-License-2014, BSD-4-Clause\n\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WriteString is cool, thanks! Please follow up with the following changes:
-
Put all the text in one multiline "raw string" (see below).
-
Please move it to a
const licenses = ...
, defined at the end of this file (see below). -
Please remove most of
up:
prefixes, keep only one on the first line (see below). -
Please also add "attribution" info for all the packages; for example, for tcell files, copy the copyright line from the header, with authorship info, like this:
Copyright 2015 The TCell Authors
Points 1-3 together mean, that the final code should start roughly like this:
const licenses = `up: Ultimate Plumber v` + version + ` https://github.com/akavel/up
Copyright 2018 the up AUTHORS
License: Apache-2.0 https://github.com/akavel/up/blob/804be3cfb8982f83e850f28bef5730672b6d04da/LICENSE
...
`
os.Stdout.WriteString("up: Dependencies:\n") | ||
os.Stdout.WriteString("up: \tTcell\n") | ||
os.Stdout.WriteString("up: \t\thttps://github.com/gdamore/tcell\n") | ||
os.Stdout.WriteString("up: \t\tLicense: Apache-2.0, ECL-2.0\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote already in #34, please provide links to the license files in github repositories of the projects. For example, I don't know what "ECL-2.0" means. I would like to have some link, like:
ECL-2.0 https://github.com/gdamore/tcell/.../LICENSE#L12345
which I could easily click and see the full text of the license.
os.Stdout.WriteString("up: \t\tLicense: MIT, MIT-feh, JSON, Xnet\n") | ||
os.Stdout.WriteString("up: \n") | ||
os.Stdout.WriteString("up: \tpflag\n") | ||
os.Stdout.WriteString("up: \t\thttps://github.com/spf13/pflag\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Please note those are not all licenses that we're using yet. If you look into the go.mod file, you can see there are a bit more (some of them are transitive dependencies). Please try using
go list
to fetch all of them (see below). -
Please also add the
versions.sh
script that you used to run go-license-detector (rename it tolicenses.sh
). I believe it should be possible for us to add some smartgo list -f
call in theversions.sh
script, to get it to automatically list all disk paths to all dependencies we're using, including the transitive ones†. Ideally, I would expect the finallicenses.sh
to look more or less like shown below:go list -f '{{ range .Deps }}{{ .Dir | println }}{{ end }}' | xargs -n1 license-detector
(not tested, so not sure if this works as-is; please explore! ❤️ )
-
Also, please add a link to the
go-license-detector
project as a comment in thelicenses.sh
script.
———
† I usually run go help list
to find out how to build the proper incantation. There seems to be type Package
described there, which contains fields Deps
and Dir
fields that may be useful to us. If that doesn't work, the -m
flag may be helpful, with type Module
containing Dir
field.
) | ||
|
||
func main() { | ||
// Handle command-line flags | ||
pflag.Parse() | ||
|
||
log.SetOutput(ioutil.Discard) | ||
if *versionFlag { | ||
os.Stdout.WriteString("up: Ultimate Plumber v" + version + " https://github.com/akavel/up\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add your email and name (or other preferred identifier) in the AUTHORS file! :) ❤️
os.Stdout.WriteString("up: \tpflag\n") | ||
os.Stdout.WriteString("up: \t\thttps://github.com/spf13/pflag\n") | ||
os.Stdout.WriteString("up: \t\tLicense: BSD-3-Clause, BSD-Source-Code, BSD-3-Clause-No-Nuclear-License-2014, BSD-4-Clause\n\n") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to os.Exit(0)
, to make it 100% clear what error code we're returning in this case, and to signal that we're doing this as a conscious decision.
Sample output: