Skip to content
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

DISCO-1604: Protocol support, salt class, metalot structure fix and logging #167

Merged
merged 8 commits into from
Oct 25, 2024

Conversation

brianbolt
Copy link
Contributor

@brianbolt brianbolt commented Oct 21, 2024

Description

  • Protocol support
    • In roo protocol is a subclass of abstract thing so did the same thing when creating Protocol object
    • Overrode save route to create and update protocol ls things
    • Added Author code type to handle adding scientist name in Protocol object.
  • Salt class
    • Basic salt class which can be initiated and saved.
  • Changed meta lot to bulk load entry to prefer parent mol by default
  • Fixed logging issue which was causing duplicate logging in python script which created their own basic logging configuration.

Related Issue

DISCO-1604

How Has This Been Tested?

Added acasclient tests for protocol changes
Verified duplicate login issue was fixed when using an external library.

@brianbolt brianbolt changed the title Disco 1604 DISCO-1604: Protocol support, salt class, metalot structure fix and logging Oct 22, 2024
Copy link
Contributor

@bffrost bffrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable, but the get_all_protocols endpoint naming compared to the contents of the response is confusing to me. Awaiting your response before approving

acasclient/CmpdReg.py Show resolved Hide resolved
acasclient/CmpdReg.py Show resolved Hide resolved
acasclient/lsthing.py Show resolved Hide resolved
acasclient/acasclient.py Show resolved Hide resolved
acasclient/protocol.py Show resolved Hide resolved
acasclient/protocol.py Outdated Show resolved Hide resolved
acasclient/protocol.py Show resolved Hide resolved
Copy link
Contributor

@bffrost bffrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the nit about an unused ACAS_LSTHING constant, this LGTM. Let's spin off the refactor to a separate PR, especially since this code is already tested through the migration.

@brianbolt brianbolt merged commit de0d461 into release/2024.4.x Oct 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants