Fork me on GitHub
Slack Archive

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

2021-12-20

Channels

#clj-commons
<
2021-12-20
>
Noah Bogart18:12:30

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.

borkdude18:12:06

I think starting with a patch is the incorrect order.

Noah Bogart18:12:37

That’s a good workflow, thank you. I’ll keep it in mind going forward

borkdude19:12:10

I think those words really resonated with me

Noah Bogart19:12:08

yeah, that makes a lot of sense

slipset20:12:23

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 🙂

borkdude20:12:18

that falls under "if we reach an agreement", if...

borkdude20:12:47

and enumerating alternatives is also in there

Noah Bogart20:12:36

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

slipset20:12:32

Which comment is this?

Noah Bogart20:12:23

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

slipset20:12:12

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.

Noah Bogart20:12:01

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

Noah Bogart20:12:05

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

Noah Bogart21:12:08

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.TransientOrderedMap

slipset21:12:01

I’d argue that’s worse 🙂

Noah Bogart21:12:46

using a private MapEntry as the “not found” value instead of coercing the transient itself as a MapEntry?

slipset21:12:33

No, that contains? throws because it’s not implementing the correct protocol.

Noah Bogart21:12:04

oh, yeah, it’s not good at all haha

Noah Bogart21:12:28

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

Alex Miller (Clojure team)21:12:56

Note that ITransientAssociative2 was added in I think 1.9, so implementing that requires Clojure 1.9+

Noah Bogart21:12:16

excellent historical information, thank you so much

slipset20:12:43

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 🙂

👍 2
borkdude20:12:09

I love doing nothing

👍 1
rickmoynihan14:01:11

Really? :rolling_on_the_floor_laughing:

borkdude14:01:50

Hey, it looks like I took 2nd of January off, didn't I.

1
slipset20:12:24

That’s why I ended up as a programmer 🙂

slipset20:12:41

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.

Noah Bogart21:12:01

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”