Clojurians Log v2
clj-commons
I opened a small PR for ordered and @borkdude was kind enough to remind me to write a good PR message, but his comment gave me pause. Is there a defined process for writing PRs for the clj-commons libraries? I didn't see anything in meta.
I recently reread Rich Hickey’s stance on patches instead of PRs and optimizing for the maintainer’s time instead of the contributor’s, so I want to make sure that as I write PRs for clj-commons libraries, I’m respecting existing conventions and the maintainers.
I think starting with a patch is the incorrect order.
This is what I recommend for my own projects: https://github.com/clj-kondo/clj-kondo/blob/master/doc/dev.md#workflow
That’s a good workflow, thank you. I’ll keep it in mind going forward
https://twitter.com/stuarthalloway/status/1063413467531624455 ;)
I think those words really resonated with me
yeah, that makes a lot of sense
Hey @nbtheduke, thanks for taking us into consideration, much appreciated. I’ve been meaning to write a blog post on the subject of how to get patches submitted (in general), but haven’t quite gotten around to it. I think @borkdudes recommendation is very valid, but I think he left out one somewhat important alternative, the alternative of doing nothing 🙂
that falls under "if we reach an agreement", if...
and enumerating alternatives is also in there
Right. I look at the code and see a comment from the original maintainer that says “This is broken” and I think, “I can fix it!” so I write the fix, but it’s probably better to start with opening an issue and saying “Is this still broken? Is the comment outdated?” etc
Which comment is this?
https://github.com/clj-commons/ordered/blob/master/src/flatland/ordered/map.clj#L161
that comment also discusses a related issue to the PR that I opened: “what should the not-found value be?“, but in all of my experimentations I couldn’t actually produce a bug for the transient ordered map, only the persistent ordered map, so I didn’t make any changes for the transient ordered map
It’s an interesting study, since it’s obviously a corner case. Not asking you to do this, but it would be interesting to see if you could write a pathalotical example that proved the comment right. That could be done in a gist and linked to in an issue description. The corner case is then a transient map which contains itself as a value. We could discuss the likelyhood of that happen and then contrasted that with the complexity of potential solutions. One solution could be to add a comment somewhere which said that contains was b0rken for transient maps which contained themselves as values.
from a “code cleanliness”/“code does the thing I intend” standpoint, I find it awkward that this is cast to a MapEntry if the given key doesn’t exist, but for whatever reason, it works. If so desired, creating a (def ^:private transient-not-found (MapEntry. (gensym) (gensym))) is easily doable but that’s well outside my original PR
oh, TransientOrderedMap doesn’t implement ITransientAssociative2 at all, so contains? and find fundamentally don’t work on it regardless of it containing itself or not
flatland.ordered.map=> (-> (transient {}) (assoc! :a 1) (contains? :a))
true
flatland.ordered.map=> (-> (transient (ordered-map)) (assoc! :a 1) (contains? :a))
Execution error (IllegalArgumentException) at flatland.ordered.map/eval8248 (form-init14525214835311855450.clj:1).
contains? not supported on type: flatland.ordered.map.TransientOrderedMapI’d argue that’s worse 🙂
using a private MapEntry as the “not found” value instead of coercing the transient itself as a MapEntry?
No, that contains? throws because it’s not implementing the correct protocol.
oh, yeah, it’s not good at all haha
I documented the issue here: https://github.com/clj-commons/ordered/issues/69 and have a branch with the described fix but didn’t want to jump the gun given the rest of the conversation about “going slowly and discussing solutions first” lol
Note that ITransientAssociative2 was added in I think 1.9, so implementing that requires Clojure 1.9+
excellent historical information, thank you so much
Yeah, but I find myself often forgetting that doing nothing is a valid alternative, with some nice tradeoffs, and besides, if doing nothing is an alternative, you’ll reach an agreement 🙂
I love doing nothing
Really? 🤣
Hey, it looks like I took 2nd of January off, didn't I.
slacker 😉
That’s why I ended up as a programmer 🙂
Back to the question at hand though. I find it fairly hard and extremely annoying to have to enumerate different solutions to a problem which I already have thought out the best solution. For me. But other people might have other use cases and priorities. This is why we need the discussions even though they’re both hard and annoying. Because for some people, it might be more important that the set fns are fast than that they behave correctly on garbage input.
Yeah, I too find it really hard, hah, but it’s a good practice and provides external documentation for discovering the “best” solution vs “the one that most readily came to my fingertips”