-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Allow unsetting schema comment using NULL #3649
Conversation
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.
As far as the behavior of msar.set_schema_description
is concerned, I'm fine with this change.
However, I think this new approach would make msar.patch_schema
function more like a "replace" rather than a "patch". For example, if I pass a patch
argument like this...
{ "name": "my new schema name" }
...then I would not expect any changes to be made to the description. But with this PR, the description would be removed. Right? That makes patching a schema work more like replacing a schema (in terms of the standard verbs we agreed on). Perhaps I should have written another test case to assert that patching can be performed partially.
I think it would help avoid bugs in the future if this PR could also modify msar.patch_schema
so that it only updates the description if the description key exists in the patch.
Whether or not you choose to make that suggested change, it would also be good to update the code comment on msar.patch_schema
, so as to clarify the new behavior of that function.
I've made your suggested change in a2529ce @seancolsen. |
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've only just skimmed the diff, but yeah @Anish9901 that looks good.
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.
Code looks fine. If the FE is okay with this behavior, so am I.
Fixes #3638
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin