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.

Sign up for GitHub

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

Jump to bottom

Provide STORE for {Class,Function}Def.name in ExpressionContextProvider #394

Merged
merged 6 commits into from
Sep 29, 2020
Merged

Provide STORE for {Class,Function}Def.name in ExpressionContextProvider #394

merged 6 commits into from
Sep 29, 2020

Conversation

cdonovick
Copy link
Contributor

Summary

closes #393 by extending ExpressionContextVisitor

Test Plan

Adds basic tests for the extension

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 24, 2020
@cdonovick
Copy link
Contributor Author

Guess I should have ran all tests not just ones I edited.

Seems like this will have more widespread effects than I have anticipated. If you would accept this PR given passing test I would take stab at updating the broken ones.

jimmylai
jimmylai suggested changes Sep 26, 2020
View reviewed changes
Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

This change looks good, just needs to fix the failing tests.
You can run tox -e autofix to fix the formatting error in lint CI job.

libcst/metadata/expression_context_provider.py Outdated Show resolved Hide resolved
@jimmylai jimmylai changed the title Issue393 Use STORE for {Class,Function}Def.name in ExpressionContextProvider Sep 29, 2020
jimmylai
jimmylai approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

Looks great!

@jimmylai jimmylai merged commit 34c1826 into Instagram:master Sep 29, 2020
@jimmylai jimmylai changed the title Use STORE for {Class,Function}Def.name in ExpressionContextProvider Provide STORE for {Class,Function}Def.name in ExpressionContextProvider Sep 29, 2020
@cdonovick cdonovick deleted the issue393 branch September 29, 2020 19:38
@cdonovick
Copy link
Contributor Author

@jimmylai Could you create a new release on pypi? Thanks.

@jimmylai
Copy link
Contributor

@zsolt plans to make another release after the pending PRs are merged.
I'm reviewing them now.

@zsol
Copy link
Member

zsol commented Sep 30, 2020

Wrong Zsolt :) but yes, I expect to make a new release tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmylai jimmylai jimmylai approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

{Class,Function}Def.name considered LOAD by ExpressionContextProvider
4 participants
@cdonovick @jimmylai @zsol @facebook-github-bot