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

implement fetch_settings to project_preferences class #306

Merged
merged 8 commits into from
Mar 3, 2024

Conversation

Tooyosi
Copy link
Contributor

@Tooyosi Tooyosi commented Feb 21, 2024

implement fetch_settings to project_preferences class

@zwolf zwolf removed their request for review February 21, 2024 22:57
Copy link
Member

@lcjohnso lcjohnso left a comment

Choose a reason for hiding this comment

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

Please add a docstring for documentation. Also consider adding usage example for the ReadTheDocs site (https://panoptes-python-client.readthedocs.io/) -- especially if there is already example usage of setting the settings.workflow_id parameter.

Copy link
Collaborator

@yuenmichelle1 yuenmichelle1 left a comment

Choose a reason for hiding this comment

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

Hey Toyosi!

I had a couple of changes requested. But I think we should revisit the merged PR: zooniverse/panoptes#4280

Readability wise, I found it confusing that your method is called fetch_settings but I'm receiving more than just the settings of the UPP. (I'm receiving the whole UPP). I found a similar issue in the corresponding Panoptes PR as well.

There is a way for us to only include attributes we want with our serializers in panoptes.
See: https://github.com/zooniverse/restpack_serializer/tree/panoptes-api-version?tab=readme-ov-file#serialization

I think let's pair next week re-working the Panoptes PR to either change naming of methods or to update the methods so that it is returning what the method name says it will return.

panoptes_client/project_preferences.py Outdated Show resolved Hide resolved
panoptes_client/project_preferences.py Outdated Show resolved Hide resolved
panoptes_client/project_preferences.py Outdated Show resolved Hide resolved
panoptes_client/project_preferences.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@yuenmichelle1 yuenmichelle1 left a comment

Choose a reason for hiding this comment

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

Awesome code changes here alongside the changes on zooniverse/panoptes#4299 look good. 👍

Not blocking, but post our convo with Cliff this morning, I think it is well worth adding documentation to our user guide as well. Within "Other examples" section. See: https://panoptes-python-client.readthedocs.io/en/latest/user_guide.html#other-examples

Additions to user guide doc can be added here: https://github.com/zooniverse/panoptes-python-client/blob/master/docs/user_guide.rst#other-examples

@lcjohnso
Copy link
Member

^^ I agree with above: please add usage example to User Guide doc pages -- both for your new fetch_settings() function and the existing save_settings() function. Ping me for a re-review when you have docs for me to look at.

@Tooyosi Tooyosi requested a review from lcjohnso March 1, 2024 15:20
Copy link
Member

@lcjohnso lcjohnso left a comment

Choose a reason for hiding this comment

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

@Tooyosi I made small edits to the user guide docs text; it is good to go now -- thanks!

@Tooyosi Tooyosi merged commit d23d2b7 into master Mar 3, 2024
2 checks passed
@Tooyosi Tooyosi deleted the UPP-fetch_settings branch March 3, 2024 14:58
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.

None yet

4 participants