-
Notifications
You must be signed in to change notification settings - Fork 146
refactor: don't set readonly by default
#377
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
Conversation
|
cc @svix-jbrown |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Probably it's overkill, creates other questions about the API around this method and requires more code to satisfy code coverage. While there are cases when someone wants to lower their rights, it's uncommon. |
readonly setter, don't set by defaultreadonly by default
|
Deleted the |
|
I want to say we should ignore the codecov failure this time. There's no way deleting code made the diff coverage go down. |
534cacb to
725562f
Compare
Summary
readonlyby default to avoid conflicts with existing user settings.Client::with_option()orQuery::with_option().closes #343
fixes #373
closes #361 (superceded PR)
Checklist
Delete items not relevant to your PR:
ExamplesIntegration test (waiting on bikeshedding)I chose the method namewith_read_onlyto match theReadOnlyenum even though the option name isreadonly. If it waswith_readonly()then for consistency the enum should be namedReadonlybut that doesn't look right. Does this make sense?Iswith_read_only/ theReadOnlyenum overkill? I imagine most of the time the reason to setreadonlyis to override what we set by default, which I'm deleting anyway as it's causing a regression (v0.14.1 is incompatible with settings profiles setting readonly=2 #373). Otherwise it's always possible to set throughwith_option/set_option.Does it make sense thatwith_read_only(false)unsets the option rather than sendingreadonly="0"? Practically speaking, there isn't really a use for sendingreadonly="0"because it's either redundant (the user/session already has read-write permissions) or not allowed (the user/session is already read-only).