-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: default predicate value on construction #159
Conversation
We could also revert 3ec1d71, keep it for a future 6.0.0 and fix it non-breaking for now and make a 5.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see this as a patch 5.0.1, since it implements the intended behavior following from the already breaking change in 5.0.0.
It is not a breaking API nor ABI change for all intended use-cases, because:
- the changed property is private to the class
- it’s a header-only class, so there is less risk of a translation unit being out of sync with the header file
- the class is used internally by modulo and is not intended to be accessed directly by any public users
- the patch comes shortly after the major update
If a user did in fact use the Predicate class in some project downstream, then there are certain scenarios under which 5.0.0 and 5.0.1 might be incompatible. With the patch comes so shortly after the main breaking change, I see very little room for confusion or issues. We can still delete the 5.0.0 release as necessary if we want to be extra safe about avoiding confusion.
This is ready then 👍 |
Description
The original problem of the parent issue is solved in cc85c64. However, as explained there is another problem which is that the initialization of the
previous_value
in thePredicate
class is not ideal and should be revised.Review guidelines
Estimated Time of Review: 10 minutes
Review by commit
Checklist before merging: