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

Use --alias key #92

Merged
merged 3 commits into from
Aug 14, 2022
Merged

Use --alias key #92

merged 3 commits into from
Aug 14, 2022

Conversation

kees-
Copy link
Contributor

@kees- kees- commented Aug 10, 2022

#91

This should work. Previously, duplicate dependencies wouldn't be an issue because the commands targeted a single possible key.
My remaining concern is this version of add-deps doesn't address possible duplicates because the location it adds to is conditional. I don't know whether the project's intent would be to ignore a dupe with the same version between :deps and an alias, warn, or remove one. So please stress test this!

@borkdude
Copy link
Contributor

Maybe check if an alias already has a :deps or :extra-deps keys and adjust the key based on that, and default to :extra-deps? Other than that, I think the code looks good. Is it possible to add a test? (Not sure if we've tested add dep otherwise).

@kees-
Copy link
Contributor Author

kees- commented Aug 11, 2022

Sure, I'll continue as I have time, likely later today

@borkdude
Copy link
Contributor

Take your time :)

@kees-
Copy link
Contributor Author

kees- commented Aug 14, 2022

Hello, long-winded response to a small issue. After adding the check you mentioned I wanted to know how all these dependency maps are dealt with.

What's informing me resolve-deps reference, reference, tools.deps code, code

The implementation leads to this general precedence when a single project file declares multiple versions of a library.

  1. alias :override-deps
  2. alias :extra-deps
  3. alias :replace-deps
  4. alias :deps (deprecated)
  5. project :deps
  6. alias :default-deps ignored unless all other coords nil

So I have learned version conflicts (outside the codebase) aren't an issue except for mental overhead, maybe more if this tool is helping you touch project files less. Plus, a lot more is possible than the 2 or 3 cases I was aware of.

--

If I start throwing in more checks, cli flags etc it feels like increased spaghetti for diminished returns.

My question for you guys as the designers and maintainers, how fine-grained should neil as a tool aim to be.

  • If you're in the situation substituting all these extra options to resolve dependencies I'm guessing you'd be editing them manually.
  • Most of the flags within an alias seem semantic for an average-scoped project, because they eventually overrule the project's deps one way or another.
  • This hierarchy is a corner case anyway

The cleanest all-or-nothing approaches are keeping it naive and letting the user handle it manually, or letting them target any flag they want. Specifying anything between those makes neil more opinionated, so I'd like to consult on where the balance is before pushing anything. My first thought before all this was to check for alias :deps, default to :extra-deps, flag to allow :override-deps for its semantic use and call it a day.

This continues to be an interesting task after getting excited for an analogue to pip install or npm install, lol.... Thanks!

@rads
Copy link
Collaborator

rads commented Aug 14, 2022

Specifying anything between those makes neil more opinionated

I agree with this. In my opinion we should keep it simple for now:

  • neil add dep with no --alias always adds to [:deps]
  • neil add dep --alias foo always adds to [:aliases :foo :extra-deps]
    • Special case: If deprecated :deps key is already present in [:aliases :foo], use that instead of :extra-deps for compatibility

For project-level deps.edn aliases, :extra-deps is almost always what the user wants. I think for other cases like :override-deps or :replace-deps it's reasonable to edit the deps.edn file by hand. Otherwise, we have a lot of options to potentially support.

This is just my feedback; hopefully it helps. I'll leave it to @borkdude for next steps here.

@borkdude
Copy link
Contributor

I agree, let's keep it simple, user can always adjust manually but let's also take into account the existing key and use that if present.

I don't think :deps is deprecated, it's just a synonym for :replace-deps. You can see it still being used in the tools build guide for example here:

{:paths ["src"] ;; project paths
 :deps {}       ;; project deps

 :aliases
 {;; Run with clj -T:build function-in-build
  :build {:deps {io.github.clojure/tools.build {:git/tag "TAG" :git/sha "SHA"}}
          :ns-default build}}}

@borkdude borkdude merged commit 44e630d into babashka:main Aug 14, 2022
@kees-
Copy link
Contributor Author

kees- commented Aug 14, 2022

Ok sounds good both replies.

I'm working on tests and was getting hung up on failures related to searching for licenses, oops.. Then checked github:

{"message":"API rate limit exceeded for ............ (But here's the good news: Authenticated requests get a higher rate limit.
Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}

Now writing test(s) for alias use cases and finding that --alias needs to be specified after --deps-file or it will target deps.edn, that might be my doing. It looks like this PR just merged so I can submit another once finalized or however is easiest

@borkdude
Copy link
Contributor

@kees- Ah right, tests welcome, merged this a bit prematurely :). Yes, this github rate limiting is annoying, but perhaps you can test with a predefined version.

@rads rads mentioned this pull request Aug 14, 2022
2 tasks
@kees- kees- mentioned this pull request Aug 14, 2022
2 tasks
@borkdude
Copy link
Contributor

borkdude commented Oct 11, 2022 via email

@rads
Copy link
Collaborator

rads commented Oct 14, 2022

Got it, thanks for the clarification.

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.

3 participants