manage all dependency settings with managed dependencies

[loomis]

Implements the ability to manage all dependency options through managed dependencies, in addition to the version. See issue #76. The implementation passes the previous unit tests and new tests have been added to show and to verify the new behavior.

[loomis]

@cemerick Hello, what is the best way to get feedback on this PR? I know that you're spending less time with pomegranate; is there someone else (or mailing list) to ask for feedback?

[cemerick]

Yeah, I'm in no position to evaluate something like this. I'm happy to have pomegranate grow features to accommodate new use cases, but the most important thing IMO is making sure that the changes are neutral to positive for established downstream consumers. A +1 from a significant contributor to e.g. Leiningen or boot would be enough for me to merge.

[cprice404]

It'd be awesome if you could make this fn public while you're in here; the fact that it is private was an oversight on my part, and led to this nastiness: https://github.com/technomancy/leiningen/blob/master/leiningen-core/src/leiningen/core/classpath.clj#L571

[loomis]

I've made merge-versions-from-managed-coords public and added a simple test. Edge cases are tested in the tests for the other functions, so didn't try to be exhaustive with this one.

I've made conform-coord and unform-coord public as well. These seem to be utilities that crop up every time someone's having to manipulate dependencies. If you don't think these are generally useful, I can make them private again.

[cprice404]

@loomis I took a first pass at looking through this code today, and it seems reasonable. A couple of things that came to mind:

1. Could you provide a bit more context as to the use case? Like, an example of a key that you want to put into the managed-coords map that wasn't successfully making it through?
2. Ultimately I think it would probably be a good idea to add another test or two that covers the actual resolution of the artifact through aether, for a case where it doesn't currently work for you. Something along the lines of this test. The tests you added seem really valuable for testing your new fns, but I'm not seeing a place in them where they trigger a full artifact resolution, unless I'm missing something?

[cprice404]

FWIW I ran leiningen's tests against this and they all passed.

[cprice404]

@hypirion I'd be curious to hear if you had any thoughts on whether this is a reasonably future-proof way to check whether a version is specified. It seems like leiningen is already somewhat committed to this approach so it's probably fine?

[loomis]

A more accurate way to do this would be to fully parse the dependency vector. Doing this by hand is a lot of work for little gain. I think the above works for all valid dependency vectors with/without versions, but if you can think of cases where it doesn't I'm happy to change this.

As an aside, I have (alpha) spec definitions for the dependency vectors. I didn't think of adding them here as they would require Clojure 1.9+. However, I can put them in if they are useful, although I'd probably need help in creating the conditional build/test with Clojure 1.9.

(ns deps.spec
  (:require [clojure.spec :as s]))

(s/def ::extension string?)
(s/def ::classifier string?)
(s/def ::scope #{"compile" "test" "provided"})
(s/def ::optional boolean?)

(s/def ::exclusion
  (s/& (s/cat :project symbol?
              :options (s/keys* :opt-un [::extension ::classifier]))
       (s/conformer (fn [{:keys [project options]}] (merge options {:project project}))
                    (fn [{:keys [project] :as m}] (let [m (dissoc m :project)]
                                                    {:project project
                                                     :options m})))))

(s/def ::exclusions (s/coll-of ::exclusion :min-count 1))

(s/def ::dep
  (s/& (s/cat :project symbol?
              :version string?
              :options (s/keys* :opt-un [::extension ::classifier ::scope ::optional ::exclusions]))
       (s/conformer (fn [{:keys [project version options]}]
                      (merge options {:project project, :version version}))
                    (fn [{:keys [project version] :as m}]
                      (let [m (dissoc m :project :version)]
                        {:project project
                         :version version
                         :options m})))))

[hypirion]

@cprice404: Yeah, this should be fine. If this changes in tools depending on pomegranate, then they can just normalize the input arguments.

[cprice404]

@loomis nice... you might be interested in technomancy/leiningen#2223, which introduces some spec stuff for the full lein project map (and does so in a way that doesn't require Clojure 1.9 AFAIK). It might be nice to try to consolidate the parts of the spec that are common - e.g. dependency vectors... and maybe pomegranate is a reasonable place to do that. (Not suggesting it would need to be done as part of this PR or anything, just food for thought :) )

[loomis]

@cprice404 Thanks for looking through the PR. I'll get back to you on the two points you raised as soon as possible (although probably not until next weekend). The behavior I'm looking for seems to be what's in the test you pointed out; however, I don't get this behavior from boot. I'll have to dig further to see where the real issue is.

[cemerick]

Ping to see where this stands. Sounds like it's still WIP?

[loomis]

@cemerick Yes, still WIP. With other urgent things, I've not been able to get back to this to complete it. Hopefully will be able to get it done in July.

[cemerick]

Just to keep myself from looking at this repeatedly, going to close now. If you ever pick this up again, feel free to reopen.