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 attribute visitors #27

Merged
merged 8 commits into from
Aug 28, 2019
Merged

Implement attribute visitors #27

merged 8 commits into from
Aug 28, 2019

Conversation

DragonMinded
Copy link
Contributor

Summary

It can be useful in a variety of situations to know what attribute of a node you are currently visiting children on. So, implement the ability to track this via attribute visit_ and leave_ support. This pull request finishes support to visit on attributes, adds a test to verify visitor behavior, and adds codegen for attribute visitor functions.

Note that this is on top of the existing pull request for codegen (#23).

Test Plan

tox

@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 Aug 14, 2019
Copy link
Contributor

@bgw bgw left a comment

Choose a reason for hiding this comment

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

All my comments are about the implementation of the codegen. Everything else LGTM.

libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/_visitors.py Show resolved Hide resolved
libcst/codegen/gen_visitor_functions.py Outdated Show resolved Hide resolved
libcst/tests/test_visitor.py Outdated Show resolved Hide resolved
@jimmylai
Copy link
Contributor

@DragonMinded, can you rebase on master so that codecov verifies the test coverage for you?

Jennifer Taylor added 5 commits August 27, 2019 14:02
…sitor.

This allows Pyre to typecheck returns inside visitors for us.
We want to make sure that the generated function stubs stay in sync with
the node definitions. So, make a unit test that fails if codegen generates
a different file than the existing file, so that somebody modifying code
knows they need to re-run codegen.
Adds a BaseStatement type which can be used to specify in types that you can
accept either a compound statement or a simple statement line.
There are a lot of nodes that cannot be removed or converted to maybes, such as
most of the Op tokens. It would be a bit of a lie to codegen leave_* methods
that allow these nodes to be converted, only to throw a runtime error later. So,
upgrade the codegen to allow us to see whether certain nodes are used in conjunction
with a MaybeSentinel/None, or inside a Sequence, to inform ourselves as to when to
allow MaybeSentinel or RemovalSentinel.
@codecov-io
Copy link

Codecov Report

Merging #27 into master will decrease coverage by 0.01%.
The diff coverage is 88.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   95.78%   95.77%   -0.02%     
==========================================
  Files         112      117       +5     
  Lines        6745    11130    +4385     
==========================================
+ Hits         6461    10660    +4199     
- Misses        284      470     +186
Impacted Files Coverage Δ
libcst/_nodes/_whitespace.py 97.77% <ø> (ø) ⬆️
libcst/__init__.py 100% <ø> (ø) ⬆️
libcst/_nodes/_op.py 95.43% <ø> (ø) ⬆️
libcst/_typed_visitor.py 96.28% <ø> (ø)
libcst/_nodes/_module.py 94.73% <ø> (ø) ⬆️
libcst/_nodes/tests/base.py 97.93% <ø> (ø) ⬆️
libcst/_nodes/_statement.py 95.98% <100%> (ø) ⬆️
libcst/metadata/tests/test_metadata_provider.py 97.34% <100%> (ø) ⬆️
libcst/_visitors.py 93.61% <100%> (+3.61%) ⬆️
libcst/_nodes/_internal.py 95.45% <100%> (+0.53%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f818773...5b43dd6. Read the comment docs.

@jimmylai
Copy link
Contributor

Can we have a follow up PR to explain the newly added attribute visitors? We can provide some examples in the Visitors page. https://libcst.readthedocs.io/en/latest/visitors.html#visit-and-leave-helper-functions

@jimmylai
Copy link
Contributor

jimmylai commented Aug 28, 2019

Question: what's the execution order of attribute functions on the same Node?
E.g. what's the order of visit_If_test, visit_If_body, visit_If_orelse, etc, when they are all implemented in a visitor?
If it's in a fixed order, please document it and add test cases for it. If we don't guarantee the order, let's also document it.

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.

Other than the comments, LGTM.

@DragonMinded
Copy link
Contributor Author

Let me put up another PR with documentation fixes, since this one is starting to get large.

@DragonMinded DragonMinded merged commit 9564606 into Instagram:master Aug 28, 2019
@DragonMinded DragonMinded deleted the visit_attribute branch August 28, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants