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

Yank scope parsing, revert to MutableScope #654

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Nov 21, 2022

After discussion of the ways in which a parse tree cannot accurately represent the meaning of consents and scopes in Auth, we have agreed to pull scope parsing from the upcoming SDK release. The code is remaining in-tree, but no longer provided via documented modules and import paths. The changelog for it has been pulled, replaced by a more modest set of changes to MutableScope, and the docs changes have been further modified to remove any references to scope parsing.

Tests continue to exercise the scope tree parsing logic and all of the utility of the Scope class as it was previously introduced.

After discussion of the ways in which a parse tree cannot accurately
represent the meaning of consents and scopes in Auth, we have agreed
to pull scope parsing from the upcoming SDK release. The code is
remaining in-tree, but no longer provided via documented modules and
import paths. The changelog for it has been pulled, replaced by a more
modest set of changes to MutableScope, and the docs changes have been
further modified to remove any references to scope parsing.

Tests continue to exercise the scope tree parsing logic and all of the
utility of the Scope class as it was previously introduced.
Copy link
Member

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

I approve of this! I left barbarically-short comments questioning if a few instances of the word "Scope" should change to "MutableScope", but this looks good to me.

src/globus_sdk/scopes/builder.py Outdated Show resolved Hide resolved
src/globus_sdk/scopes/scope_definition.py Outdated Show resolved Hide resolved
@sirosen
Copy link
Member Author

sirosen commented Nov 22, 2022

I left barbarically-short comments questioning if a few instances of the word "Scope" should change to "MutableScope",

I love that phrasing of this. 😹

The cases that you picked out were meant to be reverted to MutableScope, so 👍 to both of them. I'm pushing through that commit right now.

Co-authored-by: Kurt McKee <contactme@kurtmckee.org>
@sirosen sirosen merged commit adf1001 into globus:main Nov 22, 2022
@sirosen sirosen deleted the yank-scope-parsing branch November 22, 2022 21:44
sirosen added a commit to sirosen/globus-sdk-python that referenced this pull request Nov 22, 2022
This reverts commit adf1001.

A manual change is applied to the changelog fragment, in addition to
the revert.
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