technomancy / leiningen

Moved to Codeberg; this is a convenience mirror
https://codeberg.org/leiningen/leiningen
Other
7.29k stars 1.61k forks source link

Extend "lein change" to be able to edit versions in :dependencies and :managed-dependencies #2779

Closed ejschoen closed 2 years ago

ejschoen commented 2 years ago

Consider an application suite with parent and child projects (i.e., using lein-parent) and with a versioning policy that requires, for example, consistently bumping the patch version of the parent project and the versions a collection of libraries or applications when a change is made to a common shared library.

It was already possible to use lein change to bump the version of the parent project, and to bump the version of the reference to the parent project in the children (e.g., lein change :parent-project:coords leiningen.release/bump-version :patch followed by lein change :parent-project:coords leiningen.release/bump-version :release)

However, it was not possible to automate updating version references in the :dependencies or :managed-dependencies vectors of projects or parent projects. So, for example upon the release of a hotfix to a shared jar that's built into multiple uberjars, it was necessary to manually edit the :managed-dependency vector of the parent project to update the version of the shared jar.

This PR adds to leiningen.change/change the ability to parse the sjacket DOM for vectors of vectors, and to match on the first element of an inner vector, as in:

:dependencies [[org.clojure/clojure "1.8.0"]
               [org.clojure/tools.cli "0.4.2"]]

and by generalizing slightly leiningen.change/normalize-path to handle specifications such as:

lein change :dependencies[org.clojure/clojure] set '"1.10.1"'

The above works in :dependencies and in :managed-dependencies vectors in project.clj files, and enables developers and devops processes to script the updates to version dependencies in project files.

technomancy commented 2 years ago

Thanks for submitting this patch.

I think this functionality is a good idea, but I think we should offer a better notation for it. The problem with the proposed notation is that previously, all the arguments are just in normal Clojure data syntax, and this patch sacrifices the consistency we once had.

A better approach would be to provide the behavior you want as a function. There is no need to change the argument-parsing code at all.

Rather than this: lein change :dependencies[org.clojure/clojure] set '"1.10.1"'

We should use this: lein change :dependencies leiningen.change/set-version [org.clojure/clojure] '"1.10.1"'

Does that make sense?

ejschoen commented 2 years ago

I had implemented exactly that first. But the problem is that leiningen.change/set-version would be passed a Clojure vector rather than an sjacket DOM value. So while it was possible to edit the version of a specific dependency, the resulting project.clj lost its original formatting, and that's not OK. I didn't see any alternative but to work at the level of the sjacket DOM traversal.

I'd be open to implementing something that parses the smashed path syntax without breaking existing scenarios, which is what the current PR does. But yes, it's a different syntax. I was trying to come up with a syntax that seemed natural to a programmer, but I agree it's more Java-like than Clojure-like.

I also thought about making the syntax for the path be like :dependencies:org.clojure/clojure, but that's not really Clojure data syntax either, and the normalize-path function would have figure out to instantiate a keyword or a namespaced symbol.

Then I thought about implementing a different entry point: lein change-dependency ... but I'd end up having to reimplement a lot of lein change if I did that.

Is it true to characterize the current implementation as taking all arguments as Clojure data syntax? For example, we use this frequently:

lein change :parent-project:coords set '[group/artifact "version"]'

where :parent-project:coords isn't really normal Clojure data syntax.

technomancy commented 2 years ago

Ah, I see what you mean, yeah the distinction between raw Clojure data vs the formatting-aware straightjacket data complicates things.

Is it true to characterize the current implementation as taking all arguments as Clojure data syntax?

No, you're right--it's been a while since I'd worked in this code and I forgot that we do allow nested keywords to be concatenated together without any spacing due to limitations of how the shell passes arguments thru. So that is one concession made.

I also thought about making the syntax for the path be like :dependencies:org.clojure/clojure', but that's not really Clojure data syntax either, and thenormalize-path` function would have figure out to instantiate a keyword or a namespaced symbol.

I actually like this a lot more! It retains the existing concession of leaving out the space, but if you just imagine the space is there, it lets you pretend that the :dependencies list is a map rather than a vector.

In fact; this is legal in project.clj; the dependencies are typically given as a seq of vectors because ordering is important, but if you don't care about ordering, Leiningen will still accept a map since calling seq on that map will get you a list of vectors anyway.

  :dependencies {org.clojure/clojure "1.10.1"                                                                                                       
                 ring "1.8.2"}

The only downside is that you have to encode in the change task that certain keys are treated specially. But this goes "with the grain" of the data given the relationship between maps and vectors, more than the proposed syntax IMO.

ejschoen commented 2 years ago

OK. The smashed keyword syntax now works:

lein change :dependencies:org.clojure/clojure set '"1.10.1"'

The update-setting function now determines whether the sjacket DOM location corresponding to the current path element is a map or vector, and interprets the next path element accordingly.

As with nested maps, for completeness, the code can now append a dependency for a group/artifact that isn't in the dependency or managed-dependency vector when used with the set function.

ejschoen commented 2 years ago

Formatting cleaned up. Test added for dependencies with e.g., exclusions, and update-setting refactored to use a polymorphic helper function that dispatches based upon whether the path descends into a map or vector.

technomancy commented 2 years ago

Thanks; that's much better!