-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Refactor CLI #63
Refactor CLI #63
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.
I like. Tried it out and works for my use case.
args))) | ||
|
||
(= flag "--check-main") | ||
(check-main-class args)) |
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.
For non-optional, positional arguments like this, probably a non --
form would be more common, like <cmd> jar
, ...
But since these flags are not public API, it doesn't really matter, right?
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.
Even if the flags are private, makes sense to be consistent, I'm changing it.
README.md
Outdated
nix run github:jlesquembre/clj-nix#deps-lock -- --exclude-alias test | ||
``` | ||
|
||
There is also a `--include-alias` option, to include only certain aliases. |
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.
The implemented options are the other way round from what's documented here:
Unknown option: :exclude-alias
deps-lock usage:
--deps-include List of 'deps.edn' files to parse. All files are included by default.
--deps-exclude List of 'deps.edn' files to exclude.
--alias-include List of aliases to include. All aliases are included by default.
--alias-exclude List of aliases to exclude.
--bb Include dependencies in bb.edn files.
--lein Include Leiningen dependecies.
--help Print help and exit.
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.
🤦 Good catch, thanks!
src/cljnix/core.clj
Outdated
:escape-js-separators false)] | ||
(->> (sh/sh "jq" "-n" "--argjson" "data" lock-str "$data") | ||
:out | ||
(spit "deps-lock.json"))) |
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.
if a flake.lock
isn't on the git index, it is directly git add
ed when it's generated, so that it can be picked up right away. maybe it would be worth doing the same here, to avoid confusion?
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.
Good point, I'm doing that.
@bendlas thanks for taking time to review it, useful suggestions. I'm adding some commits to address your feedback |
Split CLI in 2 binaries,
deps-lock
(public) andclj-builder
(internal)deps-lock
now has more options, to include/exclude files/aliases and to supportbb.edn
files. Initial work was done at #54@bendlas I renamed the
--ignore-alias
to--exclude-alias
Closes #37 #53