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

Introduce parentheses expression node #71

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

haya14busa
Copy link
Member

@haya14busa haya14busa commented Nov 23, 2017

This p-r indroduces s:NODE_PARENEXPR node which represents (...) expression.

Prior to this change, vimlparser completely dropped (...) data, so we cannot know binary expression is surrouneded with () or not.
For exampele, if we want to write printer of Vim AST, 1 + 2 * 3 becomes (1 + (2 * 3)) ref: #70

One big problem is that it's breaking feature.
Existing vimlparser users have to update to handle new node.

At first i thought we can add a flag to enable it, but on second thought,
it's not good to change how vimlparser works by flag and it will slows down future development.

Instead, maybe we can notify vimlparser users to update script and/or make p-r for this change.
Fortunately, it will be really easy changes.

What do you think?

@tyru
Copy link
Member

tyru commented Nov 23, 2017

it's good to change how vimlparser works by flag

👍

@haya14busa
Copy link
Member Author

Oh, i'm sorry, i mistyped. I mean it's not good (≒ bad) to use flag to change the behavior. I edited the original comment.

@tyru
Copy link
Member

tyru commented Nov 23, 2017

oh, okay.

it's not good to change how vimlparser works by flag and it will slows down future development.

What "it will slows down future development" mean?
Are you concerned about parsing time gets slow?

Hmm, I have no idea yet to tell vimlparser to parse differently.

@haya14busa
Copy link
Member Author

Hmm, I have no idea yet to tell vimlparser to parse differently.

I mean, "parsing differently" itself is bad idea. What will we do when adding another node in the future? adding yet another flag? How about tests? Do we have to test all possible option set? Maybe not.

Instead, I propose that we give up preserving backward compatibility. and notify vimlparser users to change their code or/and I'll make pull-request to their repository. (vim-lint, vint, etc...)

In addition to it, I propose that making vimlparser https://github.com/vim-jp/vital.vim compatible interface to avoid compatibility issues.

@tyru
Copy link
Member

tyru commented Nov 24, 2017

hmm, I see.

I'm just planning to vitalize vimlparser.
Fortunately, vimlparser code is well organized so it's not difficult to generate vital interface, I think.

However, I cannot understand why creating vimlparser interface in vital avoids compatibility issue.
The vital interface modifies AST to keep backward compatibility after parsing?

@haya14busa
Copy link
Member Author

for example, vim-vimlint uses vim-jp/vimlparser and once we push this change, vimlint will break instantly.
A lot of users uses vim-vimlint in CI service and it suddenly fails.

If vimlint use vimlparser through vital.vim, this problem won't happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants