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

Revise handling of multiple reports per comment #45

Open
jfroelich opened this issue Dec 31, 2020 · 9 comments
Open

Revise handling of multiple reports per comment #45

jfroelich opened this issue Dec 31, 2020 · 9 comments
Labels
bug Something isn't working

Comments

@jfroelich
Copy link
Owner

jfroelich commented Dec 31, 2020

Each time there is a linting error we generate replace operations. Each replace mutation causes other errors to go away or introduces new errors. I am not totally clear on this, but I think this means that eslint's maximum application threshold of 10 means there can only be 10 repeated applications of the rule. This works for small comments, but once a comment has several lines, and one of the earlier lines is modified, causing a cascade of changes, I believe we eventually hit the 10 limit, and eslint stops. So it leaves a comment in a partially corrected state.

Re-applying the fix again will fix the problem. But this is really not ideal. It should all happen in one fix pass.

So maybe we have to regenerate the entire comment (or comments in the case of line) as one fix? We cannot rely on ripple effects and repeated application?

One thing we could do, is only report the first error per comment, but still report errors for other comments. Maybe that would help?

@jfroelich jfroelich added the bug Something isn't working label Dec 31, 2020
@jfroelich
Copy link
Owner Author

Idea: revise the line iteration code. we still want it to be a one pass approach, e.g. online. what we want to do is identify abstract line groups. a line group is either a single block comment or a collection of adjacent single line comments. then, instead of operating on two lines at a time, merge and split should operate on line groups. this way merge/split can be revised to operate on any number of lines in a group.

@jfroelich
Copy link
Owner Author

Maybe it is working and this is a false positive, there was a bug with parsing jsdoc that may have been the cause of the problem.

@jfroelich
Copy link
Owner Author

I think i see the problem. The problem is that a line is involved in more than one report, because there are overlapping report location ranges.

@jfroelich
Copy link
Owner Author

i do not have the right fix for ranges, but it is indeed something with the ranges, because if i make a few tweaks then all fixes get applied. but what is happening right now is still some kind of overlap

@jfroelich
Copy link
Owner Author

i think i removed the range issue, but now there is a separate problem, it is almost like the reports are evaluated in a different order than reported.

jfroelich added a commit that referenced this issue Jan 3, 2021
sort of address a problem with how fixes are reported per block comment that was causing sequential reflow to work incorrectly sometimes. this is not a true fix but it reduces the impact of the incorrect logic.

progress on #45
@jfroelich
Copy link
Owner Author

eslint/eslint#7348 interesting discussion

@jfroelich jfroelich pinned this issue Jan 4, 2021
@jfroelich
Copy link
Owner Author

it is either the overlapping ranges, or the fact that the rule is generating multiple reports for a token and therefore the reason it is stopping is because we are hitting the 10 tries limit in the loop. it may make sense to do an entire rewrite anyway. i am getting the sense while reading through the whole of the eslint source code that it is re-evaluating per fix. a single comment reflow right now could generate a hundred fixes. that is pretty bad. maybe we should just be generating either one report per block comment or one report per group of line comments, and in this report we should be replacing the entire block or line group.

@jfroelich
Copy link
Owner Author

jfroelich commented Jan 4, 2021

side note: location is indeed unrelated to ranges. it is completely unclear from the documentation but when they talk about ranges they are talking only about the range the fix operates on. also, the fixes are all basically needless decoration of a simple object that contains a range property and a text property. when they talk about ranges they are talking about the range property of the fix.

@jfroelich
Copy link
Owner Author

a second thing to play around with, before a rewrite, is to consider performing all splits before merges. right now it is awkwardly interleaved per line.

@jfroelich jfroelich linked a pull request Jan 4, 2021 that will close this issue
@jfroelich jfroelich unpinned this issue May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant