Code review comment for ppa-dev-tools:add-ppa-credentials-command

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

A few minor comments and suggestions to improve before merging as we might never look back to it otherwise.

The "allow argument to specify output file" seems so easy that it should IMHO be added.
I mean any snap magic that needs to be research, yes that is later.
But allowing where to store, that should be so easy that we should not hold back on that.

And TBH a global arg (not per subcommand, but generally available) to pass a filename.
"--use-credential" (if no arg given also using the default path) and "--use-credential foo" which would do not more but load that into the environment variable - that also seems rather close. Like <1h of work.
So again, unless you already released by time pressure, please consider adding that right away.

review: Needs Fixing

« Back to merge proposal