-
Notifications
You must be signed in to change notification settings - Fork 0
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
Comments
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. |
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. |
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. |
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 |
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. |
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
eslint/eslint#7348 interesting discussion |
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. |
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. |
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. |
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?
The text was updated successfully, but these errors were encountered: