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

Added basic support for TokenVisitor and TokenTransformer (#389) #416

Closed
wants to merge 1 commit into from
Closed

Added basic support for TokenVisitor and TokenTransformer (#389) #416

wants to merge 1 commit into from

Conversation

MegaIng
Copy link
Member

@MegaIng MegaIng commented Jul 27, 2019

No description provided.

@erezsh
Copy link
Member

erezsh commented Aug 10, 2019

Thanks for this PR, and sorry it took me a while to get to it.

It looks really good, except for:

  1. There is no test to prove that it actually works, and to try to catch edge cases

  2. I think a better interface would be to include it in the regular Transformer and Visitor, and allow to initialize them with tokens=True. It will be less lines of code, will minimize the amount of classes the user needs to know or import, and better allow for future support in providing that transformer to Lark (useful for performance reasons)

@MegaIng
Copy link
Member Author

MegaIng commented Aug 10, 2019

It's ok, I am currently ignoring my responsibilities over at regex-intersections...

To your points:

  1. Yes, I will add a few tests. What edge cases can you thing of? I can currently only think of %ignored and '_IGNORED' cases, which are both kinds of token that can not possibly be handled.
  2. What do you mean by initilization? __init__? I would not do that because transformer might be initialized at different points in your code. * We could just integrate the API completely into the Transformer. Because we require UPPERCASE function-names, they can not collide with rules. We will just have to think about the subclasses.

* This would also be the fourth way how you customize your Transformer/Visitor:

  1. Which superclass do you choose?
  2. Which function to you implement? (including __default__)
  3. Which class/function-decorators do you use?
  4. (new) Which arguments do you pass to __init__?

I already think there are enough places to search for the feature you need. (and there are still some missing, e.g. python keywords as rule names)

@erezsh
Copy link
Member

erezsh commented Aug 11, 2019

  1. Just empty trees, throwing Discard, that sort of stuff. There's probably not a lot of room for edge cases here.

  2. I don't see why having 4 different "options" for visitors make it worse than 3. That's like saying the Python's class has inheritance, globals, methods, decorators, etc., so let's not add the super() keyword

I would unite Visitor and Transformer if I could, but they operate very differently. But callbacks for Tokens are a minor addition, and breaking it into new classes seems more clunky to me.

@MegaIng
Copy link
Member Author

MegaIng commented Aug 11, 2019

My argument for 2. was that we don't need an option for Token: since we use uppercase-function names, we can just integrate them.

@erezsh
Copy link
Member

erezsh commented Aug 11, 2019

@MegaIng You're right, but

  1. It might degrade performance, because it will require an attribute check for every Token, and there would be a lot of them (probably 3 to 5 times as much as tree branches, on average)

  2. It might break transformers for existing users, who used upper-case methods because they are "safe", but happened to match a token name

So my plan is to add it as an option, and in the next major version (which isn't far away) to swap the defaults, so it would be "always on" by default, but those who care about performance can turn it off.

@erezsh erezsh closed this Aug 22, 2019
@erezsh erezsh reopened this Aug 22, 2019
@erezsh
Copy link
Member

erezsh commented Sep 6, 2019

Added the ability to visit tokens in transformers into master. I used your PR as inspiration, so thanks!

@erezsh erezsh closed this Sep 6, 2019
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.

None yet

2 participants