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

Fix: Revert setting node.parent early (fixes #9331) #9336

Merged
merged 1 commit into from Sep 21, 2017

Conversation

not-an-aardvark
Copy link
Member

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This reverts commit 1488b51. Some rules
from plugins were relying on the behavior that a a node.parent
property is only sometines present, and other rules were relying on the
behavior that the AST contains no cycles when rules are created.

I think we should plan to make this change next major release, since I still think it's an improvement over the existing rule API.

Is there anything you'd like reviewers to focus on?

Nothing in particular

This reverts commit 1488b51. Some rules
from plugins were relying on the behavior that a a `node.parent`
property is only sometines present, and other rules were relying on the
behavior that the AST contains no cycles when rules are created.
@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features labels Sep 20, 2017
@eslintbot
Copy link

LGTM

@not-an-aardvark not-an-aardvark added the regression Something broke label Sep 20, 2017
@ilyavolodin
Copy link
Member

Should we release it as a patch release? We haven't merged in anything yet that would prevent us from doing it tomorrow.

@not-an-aardvark
Copy link
Member Author

Seems reasonable to me. If we're releasing it tomorrow we may want to reopen the release issue and add another comment to prevent people from merging anything else.

@ilyavolodin
Copy link
Member

Sounds good. I'll reopen release issue.

@ilyavolodin ilyavolodin merged commit 4f87732 into master Sep 21, 2017
@not-an-aardvark not-an-aardvark deleted the revert-parent-change branch September 21, 2017 20:14
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 21, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features regression Something broke
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants