Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

not-after? not semantically equivalent to <= despite documentation implying otherwise #98

Closed
nessdoor opened this issue Apr 22, 2023 · 5 comments

Comments

@nessdoor
Copy link

The documentation for not-after? recites:

Like before?, except returns true if the inputs are equal.

The documentation for before? states:

Returns true if time entities are ordered from the earliest to the latest (same semantics as <), otherwise false.

This seems to imply that not-after? should be semantically equivalent to <=, but it is not when invoked like:

(not-after? i)
=> false

If semantically equivalent, this s-expression should have returned true. Note that before? is, in fact, semantically equivalent to <:

(before? i)
=> true

This issue seems to be a consequence of how not-after? is defined as the complement of after?, as the latter is consistent with the behavior of >:

(after? i)
=> true

The same applies to not-before?.

Not sure if this is intended behavior or not, but I found it rather confusing. In addition, when one of these functions is apply-ed, the client code is forced to explicitly check for the arity 1 case and override the result of the evaluation, like so:

(if (> (count coll) 1)
     (apply not-after? coll)
     true)
@frenchy64
Copy link
Collaborator

Good catch. The zero arities don't make much sense to me, but I'd rather go a deprecation route than fixing them. Perhaps we can introduce a couple of new predicates, say, chronological? and reverse-chronological? with the <= semantics?

@frenchy64
Copy link
Collaborator

@nessdoor thoughts on #99?

@nessdoor
Copy link
Author

nessdoor commented Apr 25, 2023

I think it's fine to deprecate and replace, if the current name doesn't make sense for the new semantics. But I have no experience with public library design, so I can't provide any further suggestions on that.

I will provide more feedback under the PR directly, so that we don't disperse things.

@nessdoor
Copy link
Author

Oh, right, would it be impractical to make all affected types implement ordering as part of their interfaces? Because the problem could be entirely side-stepped, at that point.

@pjstadig-sovasage
Copy link

There's another way that not-after? is broken, and that's when given more than two arguments.

(jt/not-after? (jt/local-date 2023 11 14) (jt/local-date 2023 11 12) (jt/local-date 2023 11 13))
;=> true

It seems that not-after? really only works with exactly two arguments.

(jt/not-after? (jt/local-date 2023 11 14) (jt/local-date 2023 11 12))
;=> false

This is because (after? a b c) is really (and (after? a b) (after? b c)) and simply inverting that changes it to (or (not (after? a b)) (not (after? b c))) which is not the same.

The same issue exists with not-before?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants