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

Add AccessorProvider #807

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Add AccessorProvider #807

merged 1 commit into from
Nov 10, 2022

Conversation

matthewshaer
Copy link
Contributor

@matthewshaer matthewshaer commented Nov 8, 2022

Adding a provider which can tell what accessor to use to go from the parent to that child node

Summary

I want to be able to see what accessor needs to be used to go from the parent to the given child

Test Plan

Added unit tests

@facebook-github-bot
Copy link

Hi @matthewshaer!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@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 Nov 8, 2022
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

Comment on lines +22 to +26
for f in dataclasses.fields(node):
child = getattr(node, f.name)
if type(child) is cst.CSTNode:
accessor = self.get_metadata(AccessorProvider, child)
self.test.assertEqual(accessor, f.name)
Copy link
Member

Choose a reason for hiding this comment

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

This is not a very valuable test as it just duplicates the core logic. I suppose that's true for the parent metadata provider tests too

Copy link
Contributor Author

@matthewshaer matthewshaer Nov 9, 2022

Choose a reason for hiding this comment

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

yes - that is what I based this on - what would you suggest ? Should I serialise something like a tree of accessors and confirm it gets the right serialization ? (btw I have tested the logic works in a client application)

Copy link
Member

Choose a reason for hiding this comment

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

Should I serialise something like a tree of accessors and confirm it gets the right serialization ?

If you have the time, that would be great. No big deal if not.

@zsol zsol changed the title Adding a provider which can tell what accessor to use to go from the … Add AccessorProvider Nov 10, 2022
@zsol zsol merged commit c44b182 into Instagram:main Nov 10, 2022
@zsol
Copy link
Member

zsol commented Nov 10, 2022

Thanks! :)

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.

3 participants