-
Notifications
You must be signed in to change notification settings - Fork 413
Update the project.clj
file
#655
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
leading to inability to run REPL or tests with accordance to CONTRIBUTING.md
SLF4J: No SLF4J providers were found. SLF4J: Defaulting to no-operation (NOP) logger implementation SLF4J: See https://www.slf4j.org/codes.html#noProviders for further details.
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.
Thanks for taking a look at this, it looks good generally, but I don't think we need to add clojure to the dependencies, only to remove it from the exclusions. Can you make that change?
project.clj
Outdated
:dependencies [[org.apache.httpcomponents/httpcore "4.4.16"] | ||
[org.apache.httpcomponents/httpclient "4.5.14"] | ||
[org.apache.httpcomponents/httpclient-cache "4.5.14"] | ||
[org.apache.httpcomponents/httpasyncclient "4.1.5"] | ||
[org.apache.httpcomponents/httpmime "4.5.14"] | ||
[org.clojure/clojure "[1.8,1.13)" :scope "provided"] |
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 believe this is not actually required, we only need to remove the :exclusions
bit from above.
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.
Yes, this is not required in a sense that without it everything works the same, but this is explicit and I personally find that it is usually better to be explicit. Maybe in this specific case this doesn't give too much value, I agree, since this is merely the range of supported Clojure versions and this is already set by/can be deduced from other profiles explicitly.
@dakrone Hi Lee! This one can be merged, I think. |
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.
LGTM, thanks!
Hello again!
My work on the tests for #654 revealed several issues with the Leiningen project descriptor, most notably the inability to run the tests (or even REPL) locally as specified in the
CONTRIBUTING.md
guide. This was due to a global exclusion of Clojure dependency.After thinking for a while, I came up with a pretty standard solution for libraries — to use a "provided" dependency with an explicit range of supported Clojure versions. And of course, leave everything else intact.
The output of dependency trees for all test profiles looks like this after the changes.
lein with-profile -system,-user,+1.8:dev deps :tree
lein with-profile -system,-user,+1.9:dev deps :tree
lein with-profile -system,-user,+1.10:dev deps :tree
lein with-profile -system,-user,+1.11:dev deps :tree
lein with-profile -system,-user,+dev deps :tree
(for the latest Clojure1.12.1
which is now covered a well)From all this output it is clear that:
a. The correct version of Clojure is used every time.
b. There are already some Clojure version conflicts within
1.8
(major) and1.9
(minor) in the development environment. (Which of course can be resolved by separate dev profiles for those versions, but hey, the tests pass, so everything should work without issues.)OTOH, having Clojure in
:exclusions
results in information about its versions across all dependencies wiped away. And, in my experience, it's better to have all dependency information explicitly. It usually helps later when making decisions or analyzing issues.Most importantly, now anyone can run the full test suite using the
lein all test :all
without any obstacles. 😉@dakrone Please, tell me if I'm missing something important here. Probably not, but maybe I'm out of touch with
lein
.Have a good one!
Cheers,
Mark