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

Ignore alias option #54

Merged
merged 3 commits into from
Jun 25, 2023
Merged

Conversation

bendlas
Copy link
Contributor

@bendlas bendlas commented Jun 16, 2023

Locking all aliases may not be desirable for every project,
e.g. for development-only aliases with local paths, and such.

This PR adds the ability to ignore aliases, similar to ignoring deps files.

For some use cases, the existing ignore-deps capability might be used to achive the same result as with ignored aliases, but with some tools, that's inconvenient. E.g. shadow-cljs doesn't seem to implement support for multiple overlaid deps files.

I'm still working out how to test this and add some error handling to the options parser, but wanted to get some feedback in the meantime about the options interface and if you want this PR at all.

@jlesquembre
Copy link
Owner

Thanks for this PR, that's the direction I want to go.

My plan was to re-implement the CLI to be more granular about dependencies. My idea is to have the following flags:

  • To include/exclude *.edn files: --deps-include and --deps-exclude. Those should support glob patterns.
  • To include/exclude aliases: --alias-include and --alias-exclude (in both cases, exclude options takes preference over include)
  • --bb: Include bb.edn files, Probably in the end it will be an alias for --deps-include **/bb.edn
  • --lein: Include lein files (already implemented)

Without include/exclude options, we include everything.

I'd like to hear your toughs about the flags. Anything to add or change?

I also thought about adding a library like tools.cli or babashka.cli, but maybe the way you are implementing it with loop-case is enough, what do you think?

I'm answering your other questions separately.

My question is how you want to proceed. As an initial implementation, it's good, but I want to add more functionality. For me, it's ok if you want to work on it. Depending on how much time you want to invest, you could implement it and get it merged to main, or we could merge a subset of the functionality into a branch, and I can take it from there.

@bendlas
Copy link
Contributor Author

bendlas commented Jun 20, 2023

My question is how you want to proceed. As an initial implementation, it's good, but I want to add more functionality. For me, it's ok if you want to work on it. Depending on how much time you want to invest, you could implement it and get it merged to main, or we could merge a subset of the functionality into a branch, and I can take it from there.

Thanks to how flakes do, I'm already putting this to work for my project, and so I've got no problem if you feel like taking over. I'm also happy to help with implementing after we nail down the details.

  • To include/exclude *.edn files: --deps-include and --deps-exclude. Those should support glob patterns.
  • To include/exclude aliases: --alias-include and --alias-exclude (in both cases, exclude options takes preference over include)
  • --bb: Include bb.edn files, Probably in the end it will be an alias for --deps-include **/bb.edn
  • --lein: Include lein files (already implemented)

Without include/exclude options, we include everything.

I'd like to hear your toughs about the flags. Anything to add or change?

I'm wondering about the ergonomics of wildcards: Usually, wildcards would be expanded by the shell, so in order to pass them to deps-lock, they'd need to be quoted like --deps-include '**/bb.edn' and if user forgot to quote the wildcard, results would probably be confusing.

OTOH, if the interface was designed around shell wildcard expansion, it would also result in non-standard interface: --deps-include bb1.edn bb2.edn

Though does clj-nix really need wildcard support? What use cases are there, given that the deps-lock invokation could easily be generated by nix into a command for the flake using clj-nix?

I also thought about adding a library like tools.cli or babashka.cli, but maybe the way you are implementing it with loop-case is enough, what do you think?

Using a library seems like a good idea to me. Standard interface with generated help and all ...

@jlesquembre
Copy link
Owner

Ok, you convinced me about wildcard support, let's leave it out. Anyways projects don't have that many deps files, so probably it's going to cause trouble for almost no benefit.

Using a library seems like a good idea to me. Standard interface with generated help and all ...

Any suggestion? I'm not familiar with any clojure CLI library, I like tools.cli or babashka.cli because don't have any dependencies, but I'm open to suggestions.

@bendlas
Copy link
Contributor Author

bendlas commented Jun 21, 2023

Any suggestion? I'm not familiar with any clojure CLI library, I like tools.cli or babashka.cli because don't have any dependencies, but I'm open to suggestions.

I haven't formed an opinion on CLI libs so far either. Maybe go with tools.cli by default? Try with both and compare outcomes?

@jlesquembre
Copy link
Owner

I'm thinking about merging this PR, and continuing working on the CLI on another PR, any objections?

Regarding CLI options, we can have:

  • deps-include
  • deps-exclude
  • alias-include
  • alias-exclude
  • bb (Include bb.edn deps, false by default)
  • lein (Include lein deps, false by default)

No wildcards, deps-include / deps-exclude and alias-include / alias-exclude are mutually exclusive. Without any options, all deps.edn / aliases are included. Makes sense to you? Any suggestions about the option names?

Something I forgot to mention, in the current version we also have some other flags, like --jar or --patch-git-sha. I don't like to expose those flags, I'll move those somewhere else.

About the CLI libraries, I played a bit with both, no strong opinions, but I'm liking babashka.cli a bit more, probably I'll go with that one.

@bendlas
Copy link
Contributor Author

bendlas commented Jun 25, 2023

I'm thinking about merging this PR, and continuing working on the CLI on another PR, any objections?

No objections.

I'm liking babashka.cli a bit more, probably I'll go with that one.

LGTM

No wildcards, deps-include / deps-exclude and alias-include / alias-exclude are mutually exclusive. Without any options, all deps.edn / aliases are included. Makes sense to you?

"all deps.edn" as in the current behavior? All files named deps.edn in subdirectories?

Any suggestions about the option names?

Your proposals seem fine to me.

@jlesquembre
Copy link
Owner

"all deps.edn" as in the current behavior? All files named deps.edn in subdirectories?

Yes, as in the current behavior, all deps.edn in all subdirectories

@jlesquembre jlesquembre merged commit 72de757 into jlesquembre:main Jun 25, 2023
This was referenced Jun 25, 2023
@bendlas bendlas deleted the ignore-alias-option branch August 19, 2024 11:10
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.

2 participants