-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add ConsentRequired handling to Transfer example #673
Conversation
Two new scripts are introduced: - purely reactive (like globus-cli) - best-effort proactive, which falls back to being reactive These demonstrate the current best possible experience when handling ConsentRequired errors, and have been tested against a non-HA GCSv5 Mapped Collection to verify their behaivors.
|
||
We'll also enhance the example to take endpoint IDs from the command line. | ||
|
||
.. code-block:: python |
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.
For a runnable script like this, it might make sense to push this into an actual script, particularly for the purposes of running the code to test its functionality, and running it through the full suite of pre-commit hooks.
.. literalinclude:: consent_required_reactive.py
:language: python
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 approved the PR, but still submit this feedback for your consideration. 😀
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 been meaning to try to read up on "what was the syntax for including scripts again...?"
If I can change it without breaking the doc build in < 10 minutes, I'll make the change here and include it in the PR.
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 just got back to this and pushed a small change to remake these into a set of downloadable literalinclude targets.
I'd appreciate it if you could take a gander at the result and either approve again or merge (either is fine, IMO) if it looks good!
Move the transfer examples to a dir, putting each script into a separate file, and then pulling them in with literalinclude and a caption including a download link. Although the page title is changed, the name is not (in order to preserve any links, internal or external).
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 great to me!
At some point I'd love for you to help me figure out how to access the built docs for a given branch. I haven't gotten that into my brain yet (or maybe that's for the docs site...but either way!).
If you expand the "show all checks" view on a PR, it should show links for RTD and precommit. I think there's a RTD config option for making it comment with the link, which we could turn on too. (I'd need to read up on it again to be sure; I might be thinking of someone's bot.) |
Thanks! That's what I needed to know! |
Two new scripts are introduced:
These demonstrate the current best possible experience when handling ConsentRequired errors, and have been tested against a non-HA GCSv5 Mapped Collection to verify their behaivors.