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

feat: add specifications for new operators #25

Merged
merged 7 commits into from Feb 9, 2022
Merged

Conversation

ivarconr
Copy link
Member

@ivarconr ivarconr commented Feb 2, 2022

No description provided.

@ivarconr ivarconr requested a review from chriswk February 2, 2022 10:05
schema/features-schema.js Outdated Show resolved Hide resolved
@ivarconr
Copy link
Member Author

ivarconr commented Feb 2, 2022

The specifications runs green in the node SDK implementation of them 🔥 (PR)

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value/values needs to stay a string

schema/features-schema.js Outdated Show resolved Hide resolved
{
"contextName": "currentTime",
"operator": "DATE_AFTER",
"value": "2022-01-29T13:00:00.000Z"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a minor thing but! Should DATE_BEFORE and DATE_AFTER be inclusive (i.e. LT versus LTE)?

I don't see this being a huge issue (there's a vanishingly small chance people will notice) but figured I'd ask as long as I was writing unit tests. In the Python client, it's currently coded to be inclusive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meaning of the word "AFTER" seems to be closer to GT.

For this reason I think we should require it to be exclusive.

@RikudouSage
Copy link

I'm missing some edge cases for versions, like 1.2.3-beta being lower than 1.2.3 and greater than 1.2.3-alpha.

@chriswk
Copy link
Contributor

chriswk commented Feb 7, 2022

Good catch @RikudouSage - they are indeed missing, and we should add those as well :)

@ivarconr
Copy link
Member Author

ivarconr commented Feb 9, 2022

I think we are close to a an agreeable state on the specs.

Would be good to get feedback from

Copy link
Member Author

@ivarconr ivarconr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now :)

@ivanklee86
Copy link
Contributor

Updated spec tests in Unleash/unleash-client-python#192 and everything looks good. :)

Copy link

@RikudouSage RikudouSage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, updated to latest version in Unleash/unleash-client-php#47 and everything works.

@chriswk chriswk merged commit ebc4aec into main Feb 9, 2022
@rarruda
Copy link

rarruda commented Feb 9, 2022

  • How about also allowing using DATE_GTE, DATE_GT as more precise definitions of DATE_AFTER?
    (and the equivalent for _LTE, _LE of for DATE_BEFORE)

  • It would have been useful to also have SEMVER_GTE and _LTE

  • It would also nice to have tests for other strings which are definitely not SEMVER, so we have predictable behaviour too. Maybe just do in the current locale string ordering when one of the values is not a SEMVER? (optionally log a warning?) Like :

    • "v1.0.0" <=> "1.0.0" ( I know that versions prefixed with v are not SEMVER but they are still widely use, so it would be good that we at least test for them)
    • "2.0.0-beta.1" <=> "2.0.0b1"
    • "abc" <=> "abz"
    • "1.0" <=> "abc"

I understand that this feedback might be a bit late, as the spec seems to be closed now, but I thought it was still worth giving my 2 cents.

@RikudouSage
Copy link

  • How about also allowing using DATE_GTE, DATE_GT as more precise definitions of DATE_AFTER?
    (and the equivalent for _LTE, _LE of for DATE_BEFORE)

This doesn't make much sense IMO, since the current time would need to be the same down to exactly seconds (or milliseconds, microseconds, whatever your implementation takes into consideration).

@rarruda
Copy link

rarruda commented Feb 10, 2022

@RikudouSage Given that DATE_AFTER is strictly after (exclusive), we kind of are in a conundrum anyhow:

How do you represent the from 1pm and on for a given date?

using DATE_AFTER with "2022-01-29T13:00:00.000Z" then:

  • "2022-01-29T13:00:00.000Z" will be excluded
  • "2022-01-29T13:00:00.001Z" will be included.

If you want to include "2022-01-29T13:00:00.000Z" as well, then you could set DATE_AFTER to "2022-01-29T12:59:59.999Z", but what if there is an event at "2022-01-29T12:59:59.9999Z" that you don't want included?

@rarruda
Copy link

rarruda commented Feb 10, 2022

I understand your concern regarding "exactly" equals time, but It should be safe to assume that non defined precisions are floored down to zero.

@ivarconr
Copy link
Member Author

ivarconr commented Feb 11, 2022

It would also nice to have tests for other strings which are definitely not SEMVER, so we have predictable behaviour too.

We constantly should add more edge cases as we discover them. As long as the correct behavior is possible to document and within the agreed protocol any deviation should be considered as a bug in the respective SDKs.

It would have been useful to also have SEMVER_GTE and _LTE

Maybe, we can add these later.

How about also allowing using DATE_GTE, DATE_GT as more precise definitions of DATE_AFTER?
(and the equivalent for _LTE, _LE of for DATE_BEFORE)

These date operators where on purpose given a different name, just because they are "less accurate". The chances of a date-time to be equal on a sub-second level is quite small, and only adds additional options the user needs to consider.

For DATE operators I do not worry much about micro-precision. If you have the need to be precise on this level you probably also worry about time-synchronization between servers and would probably not rely on any third part tool for your business logic.

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

Successfully merging this pull request may close these issues.

None yet

5 participants