-
-
Notifications
You must be signed in to change notification settings - Fork 169
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(postgrest): Update parameter type of match()
filter so that null values cannot be passed.
#919
Conversation
match()
to not accept null
values.
This is technically a breaking change. A map may be of type |
I think we can still consider it a fix for a type that we didn't get right in the first place. |
But the type of a map may be easily |
Fair enough 👍 |
match()
to not accept null
values. null
value is passed to match()
filter.
var url = _url; | ||
query.forEach((k, v) => url = appendSearchParams('$k', 'eq.$v', url)); | ||
query.forEach((k, v) => url = appendSearchParams(k, 'eq.$v', url)); |
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.
Might be good to undo this.
query.forEach((k, v) => url = appendSearchParams(k, 'eq.$v', url)); | |
query.forEach((k, v) => url = appendSearchParams('$k', 'eq.$v', url)); |
I like the first proposed solution better. It gives the developers type-safety, causing less hassle. It might be good to have this shipped in the next major release. |
I like it too. I think sometimes we try to be a bit too picky about what defines a breaking change. I feel like the world does revolve around these minor "technically breaking changes" being shipped as a bug fix all the time. Flutter itself ships "breaking changes" without bumping the major versions all the time, and as long as it's under moderation, I don't think we should hesitate to ship these changes. I'd say we just ship it with the original change. |
null
value is passed to match()
filter.match()
filter so that null values cannot be passed.
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 don't think you can compare Flutter breaking changes with package breaking changes. Packages should use semver, Flutter doesn't.
I'm pretty sure three are code basis, which throw a static analysis error now, which definitely is a breaking change. But sure, our other bug fixes may sometimes be more breaking, and they don't even show up when compiling.
I wouldn't make this breaking change, but you decide.
What kind of change does this PR introduce?
Fixes #917
What is the current behavior?
Currently, users can pass
null
tomatch()
filter.What is the new behavior?
Users will not be able to pass
null
tomatch()
.