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 a bug that disabling directives are not properly applied in some cases #312

Merged
merged 2 commits into from
Aug 18, 2019

Conversation

kiyot
Copy link
Contributor

@kiyot kiyot commented Aug 12, 2019

This PR fixes #303

Node#line_numbers includes line numbers of child nodes. That causes RootNode#node_for_line to return an ancestor of the node containing the specified line.

Directives are applied based on RootNode#node_for_line, so there are cases like #303 where directives are not properly applied.

Concerns

This PR reverts some part of #278, which is about multiline node handling.
I believe child nodes are not meant to be handled like multilines, and this PR should be okay.

kiyot added 2 commits August 12, 2019 23:09
Node#line_numbers includes line numbers of child nodes.
That causes RootNode#node_for_line to return an ancestor of the node containing the specified line.

Directives are applied based on RootNode#node_for_line,
so there are cases like sds#303 where directives are not properly applied.
@sds sds added the bug label Aug 18, 2019
@sds sds merged commit b1c26b8 into sds:master Aug 18, 2019
@sds
Copy link
Owner

sds commented Aug 18, 2019

Thanks @kiyot. This issue seems to be affecting a decent number of folks, so appreciate a push in a direction that alleviates, though I'm not sure about this statement:

...child nodes are not meant to be handled like multilines, and this PR should be okay.

I don't have a good sense of whether this is true/false, but I accept that an attempt at a fix is better than no action. Thanks again.

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

Successfully merging this pull request may close these issues.

Since version 0.30, disabling LineLength doesn't work.
2 participants