-
Notifications
You must be signed in to change notification settings - Fork 16
Issue 48490: Add auditing behavior param via new options param #66
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
I considered updating select_rows() to take in an 'options' param too rather than the list of ~14 params and editing up the fn so that the payload is still well-formed, but it seemed likely unnecessary and backwards incompatible. Waffling though so happy to do so if we think it's a good idea. |
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 am not 100% sure about the options dict approach, I have the same issues with it as you. I pinged Nick to get his thoughts, but I am probably leaning towards just adding them all as kwargs in order to better match our JS API.
Additionally, we should make some small changes to keep the supported Python versions to 3.7+.
Co-authored-by: Alan Vezina <alanv@labkey.com>
ToDo:
|
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.
Looks good. You'll need to merge Nick's changes in before merging. He bumped the major version number, but I haven't released that yet, so all you'll need to do is put your release notes under the same version as Nick.
Manual Test Plan
Validate the difference in response lengths of resp1 and resp2.
Validate the audit log comments and user comments.
Validate audit log comment and user comments. |
Rationale
See more details here. It is desired we add support for a couple of options passed to insert_rows, update_rows, and delete_rows. There are a notable amount of parameters that are supported on the server but not passed to these functions (around 8). As such, it seemed more maintable and elegant to add an 'options' paramater to these functions rather than passing all ~8 explicitly.
Related Pull Requests
Changes