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

SYNPY-316: Deprecate getProvenance and create get_activity #980

Closed
wants to merge 3 commits into from

Conversation

BryanFauble
Copy link
Contributor

Problem:
getProvenance is not a term defined in the REST docs - should be getActivity

Solution:

  1. Create the new get_activity function and deprecated the old function
  2. Updated all code I could find to use get_activity instead of getProvenance

Testing:

  • I an all unit testing, relying on github pipeline for Integration testing

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

I know this is in draft mode, but here's some initial comments

synapseclient/client.py Show resolved Hide resolved
synapseclient/client.py Show resolved Hide resolved
@@ -1483,7 +1493,7 @@ def build_parser():
type=str,
help="Output the provenance record in JSON format",
)
parser_get_provenance.set_defaults(func=getProvenance)
parser_get_provenance.set_defaults(func=get_activity)
Copy link
Member

Choose a reason for hiding this comment

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

We may have to add another parser so that people know that the cli

synapse get-provenance

is going to be deprecated. As it is written right now, the deprecation warning won't ever be triggered and we don't have synapse get-activity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - I will add in get-activity for the CLI.

What is the way to mark these are deprecated? I can add information into the help message like:
help="show provenance records. This is *deprecated* - Swap to using get-activity instead."

But is that sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

If you set the default function for get-provenance to be getProvenance, is it able to leverage the deprecated decorator?

Also - that might not be enough, so we could intentionally add a log.

Copy link
Contributor Author

@BryanFauble BryanFauble Oct 11, 2023

Choose a reason for hiding this comment

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

ok great - I am going to leave this calling getProvenance. In the console it prints out the warning:

DeprecationWarning: Call to deprecated function (or staticmethod) getProvenance. (deprecated and replaced with `get_activity`) -- Deprecated since version 3.1.0.

@pep8speaks
Copy link

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1436:89: E501 line too long (103 > 88 characters)
Line 1535:89: E501 line too long (101 > 88 characters)

@BryanFauble
Copy link
Contributor Author

Closing PR for now - Will re-open at later date when thinking about jira: SYNPY-1303

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.

3 participants