dm3 / clojure.java-time

Java 8 Date-Time API for Clojure
MIT License
471 stars 47 forks source link

Fix `not-{before,after}?`, introduce `<`, `>`, `<=`, `>=`, `+`, `-`, `neg?` #99

Closed frenchy64 closed 1 year ago

frenchy64 commented 1 year ago

Close #98

nessdoor commented 1 year ago

I think that chronological? and reverse-chronological? could be a little bit of a mouthful. Maybe it would be better to shorten them to something like chrono? and rev-chrono?. Also, I don't think "reverse" is the right word, but rather "inverse" (so inverse-chronological? or inv-chrono?)?

Finally, as for the implementation itself, I guess it wouldn't be practical to directly convert time quantities to numbers and then compare them through regular comparison predicates, right?

frenchy64 commented 1 year ago

@nessdoor my intention was to imply the concepts of "chronological order" and "reverse-chronological order". Not sure if it's a great mapping since "before?" takes into account both the start and end of intervals, and usually chronological ordering only concerns the start. (If we go with "chronological", I think there's too many usages of "chrono" and "chronology" in this lib and java.time to shorten it without losing clarity).

Can you think of any other concepts that might be appropriate? Perhaps just before-or-equal?? Or before-or-abuts?.

nessdoor commented 1 year ago

I disagree that chronological ordering only concerns the start of intervals, but rather that the order relation becomes partial when intervals are thrown into the mix.

As for the names, it's difficult to come up with something. Maybe not-earlier? and not-later?? But then again, it sounds good for an arity of 2, but becomes a little bit of a stretch with arities 1 and 3+. What do you think?

Naming things is so difficult... Maybe just correcting the implementation without introducing new predicates is still an option? Or do you think that this risks breaking code that depends on it, and is therefore unfitting for a minor release?

frenchy64 commented 1 year ago

I disagree that chronological ordering only concerns the start of intervals, but rather that the order relation becomes partial when intervals are thrown into the mix.

Ok, well that was my main reservation. How about chrono-order? and rchrono-order? (a la rseq).

Could you also elaborate on why you prefer "inverse chronological" over "reverse"? The latter is native to me as an Australian, but I prefer US idioms in Clojure code.

As for the names, it's difficult to come up with something. Maybe not-earlier? and not-later?? But then again, it sounds good for an arity of 2, but becomes a little bit of a stretch with arities 1 and 3+. What do you think?

Personally I dislike not-* or unless-* naming in terms of readability, I'd rather an non-negated name. I like your insight about non-2 arities, that seems to be a big part of the cognitive dissonance in this case.

I find it cognitively difficult to map not-after? to <= (did I get it right?).

Naming things is so difficult... Maybe just correcting the implementation without introducing new predicates is still an option? Or do you think that this risks breaking code that depends on it, and is therefore unfitting for a minor release?

Primarily the latter. Also I did not write the implementation, and it's so terse and old that I don't feel comfortable making too many assumptions about its intention and usages.

After reflection, it's also a good opportunity to deprecate a badly named function.

nessdoor commented 1 year ago

chrono-order? and rchrono-order? sound good. I can think of other two options, namely chrono-ordered?/rchrono-ordered? and chrono-ord?/rchrono-ord?, depending on the verbosity level you prefer.

As for the "inverse" vs "reverse", I heard the former more often than the latter in mathematical discourse (and it seems to be used when talking about order theory). Nonetheless, not being a native English speaker myself, I cannot say I can properly detect any subtle difference in meaning between the two. The choice is up to you.

As for the rest, I agree with your opinion.

pjstadig-sovasage commented 1 year ago

I think chronological? and reverse-chronological? sound fine to me. It's possible that people may be using this not-after? and would be broken by fixing it, but I suspect people have mostly worked around it by not using it (as is my case), so I'd also be happy with fixing not-after? and not-before?.

pjstadig-sovasage commented 1 year ago

Also, unless I'm reading things wrong, there are only tests with two arguments, and it might be good to add some three argument cases?

frenchy64 commented 1 year ago

I'm also leaning towards just fixing the original functions after your report.

frenchy64 commented 1 year ago

https://github.com/dm3/clojure.java-time/commit/d2b5668601ebcdf4cb6e3551c88fb6697d4b9f38

pjstadig-sovasage commented 1 year ago

Great! Thanks for all your work