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

Feature: add support for strict/loose conditions #3

Merged
merged 2 commits into from
Mar 27, 2023

Conversation

JasonTheAdams
Copy link
Contributor

@JasonTheAdams JasonTheAdams commented Mar 24, 2023

While integrating this I came to find that making sure field types are always set is tricky. It especially gets funky when comparing things like integers and floats. After some reflection, I realized that when it comes to fields and basic comparisons, most of the time an equivalent (loose) comparison is more intuitive.

Fact of the matter is, the system as it is isn't even consistent. Our equivalence operators (== and !=) were strict, but ranger operators (>, >=, <, <=) are loose in PHP. So depending on what operator was used it was strict or loose. Hmm.

This PR makes conditions loose by default and adds a FieldCondition::strict() method to allow for a "strict mode" to be activated for a given field. It then adds additional checking in strict mode to make all operators type-strict.

My only internal debate for this is whether "strict mode" should be a property of the condition or the condition set. It feels like a $set->passesStrictly() would be kind of neat. But that would mean its not possible for a set to be mixed, and it feels like the condition itself should determine that (because, well, that is how conditions work).

Also, I did think about adding more operators, but I decided that's more confusing as an API, especially when it comes to range operators. I think 95% of the time folks will want loose operators, and this keeps things simple.

Open to suggestions, but I do think adding a strict mode in some regard is valuable, bringing consistency to the framework, and I think it should be loose by default.

@JasonTheAdams
Copy link
Contributor Author

@jonwaldstein or @borkweb, I'd appreciate a review on this. It's a blocker so I do need to make a decision and merge this with some urgency.

Thank you!!

Copy link
Member

@borkweb borkweb left a comment

Choose a reason for hiding this comment

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

I like your reasoning and the implementation looks good. 🥇

@JasonTheAdams JasonTheAdams merged commit ebc6905 into develop Mar 27, 2023
@JasonTheAdams JasonTheAdams deleted the feature/strict-comparison branch March 27, 2023 15:02
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.

2 participants