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

Support standard library (Attempt 3) #109

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented Sep 16, 2024

This attempt uses the pkg() function proposed in #101.
This changes should probably be squashed since a lot of the commit messages are irrelevant.

For the record:

  • Attempt 1 was comparing the type as strings with if node.__class__.__name__ == <str>.
  • Attempt 2 was dynamically determining the origin ast module only at strategics place and then storing it in a self.ast variable.
  • Now we're dynamically determining the origin module on the fly when needed.

But in any case some methods needs to be added to the DefUseChains class and a couple of places needs to be adjusted to cope with structural differences in between ast and gast, for instance the fact that expect handlers name are stored as Name instead of str and arg doesn't exist in gast.

@tristanlatr
Copy link
Contributor Author

Idk why the tests failed, it works on my local computer and it looks like I can’t access the log of the GitHub CI. Can you help me @serge-sans-paille ? Thanks

@serge-sans-paille
Copy link
Owner

I really like where it's going! Some comments inline though

return self.node.name
elif _PY310PLUS and isinstance(self.node, ast.MatchMapping) \
and self.node.rest:
return self.node.rest

Choose a reason for hiding this comment

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

This is the place I like the less, because we clearly loose the generic aspect of gast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use __class__.__name__ == str for these case to keep it generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @serge-sans-paille, I’ve applied the suggested changes :)

beniget/beniget.py Outdated Show resolved Hide resolved
beniget/beniget.py Show resolved Hide resolved
beniget/beniget.py Outdated Show resolved Hide resolved
@@ -559,17 +601,76 @@ def process_annotations(self):
cb(visitor(annnode)) if cb else visitor(annnode)
self._scopes = currenthead
self.defs = compute_defs

def _support_stdlib(self):

Choose a reason for hiding this comment

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

Question: should / could that be put in a subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way simplifies the usage as well as the testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But to answer your question: yes it could be a subclass. But there is no value for it to be put in a subclass instead of this.

@tristanlatr
Copy link
Contributor Author

Grand merci pour ton feed-back @serge-sans-paille !
Je vais ajuster les deux trois petits éléments cette semaine.
D’ailleurs pourrait tu me donner ton avis, ne serait-ce qu’une vague opinion, sur le ticket #55 ? Merci et bonne soirée

@serge-sans-paille
Copy link
Owner

I'm good with this! can you rebase & squash into a nice stack and we're done :-)

@tristanlatr
Copy link
Contributor Author

Could you just use the « squash and merge » option of GitHub ? Thanks

@serge-sans-paille
Copy link
Owner

serge-sans-paille commented Oct 13, 2024 via email

@tristanlatr
Copy link
Contributor Author

It looks like there are no conflicts… I don’t get it

IMG_1254

@serge-sans-paille
Copy link
Owner

Manually rebased, squashed and merged as of de126b0

@tristanlatr
Copy link
Contributor Author

Fantastique, merci

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