Bugzilla – Full Text Bug Listing
| Summary: | [quickfix] Provide quickfix for gratuitous brackets | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Steve Northover <steve_northover> |
| Component: | JS Tools | Assignee: | Curtis Windatt <curtis.windatt.public> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P2 | CC: | curtis.windatt.public, Michael_Rennie |
| Version: | unspecified | ||
| Target Milestone: | 12.0 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
|
Description
Steve Northover
(In reply to Steve Northover from comment #0) Removing brackets is safe (it we do it right) It's worth noting that the correctness of the fix is 100% dependent on the rule, and we take this rule implementation from upstream ESLint. Our tests do not even come close to testing all the cases this rule has to get right. Personally I'd like to see this rule be off by default as it marks extra parens too aggressively and there isn't a quickfix to 'ignore' the extra parens that make the code more legible. Implementing a quickfix to remove the parens will require modifying the rule code. The rule implementation marks the entire expression not just the extra parentheses. The rule would have to track which parentheses are the problem. (In reply to Curtis Windatt from comment #2) > Personally I'd like to see this rule be off by default as it marks extra parens too > aggressively and there isn't a quickfix to 'ignore' the extra parens that make the > code more legible. I agree 100%, I have removed tagged parens and it broke the logic of an expression. > Implementing a quickfix to remove the parens will require modifying the rule > code. The rule implementation marks the entire expression not just the > extra parentheses. The rule would have to track which parentheses are the > problem. We can look up the tokens at the start and end of the annotation, we don't need to track them. (In reply to Michael Rennie from comment #3) > We can look up the tokens at the start and end of the annotation, we don't > need to track them. We would have to look up the tokens before and after the annotation. It apepars it usually marks the entire expression, but not the brackets it considers unnecessary. Hopefully this is true for all cases, but the rule might have some edge case where the whole line is selected. (In reply to Curtis Windatt from comment #4) > (In reply to Michael Rennie from comment #3) > > We can look up the tokens at the start and end of the annotation, we don't > > need to track them. > > We would have to look up the tokens before and after the annotation. It > apepars it usually marks the entire expression, but not the brackets it > considers unnecessary. Hopefully this is true for all cases, but the rule > might have some edge case where the whole line is selected. No, the rule is very simple in its marking, it always tags the entire offending node immediately inside the parens. Two calls to finder#findToken should suffice. Let's change the default for this rule to be off if we cannot implement a quickfix easily and safely. Alternately, it makes us look smart because we are changing code and making it *better* without affecting behavior. Personally, there are a few cases where I would leave extra parens in but in most cases, I would want them out. Some day (not today), it'd be great to mark the ones I want to leave alone so I don't see the warning. In the very very short term, it is easy enough to change the demo code not to have this pattern. That's another approach. Please fix. I'm close to a fix on this, just need to write more complete tests and deal with an edge case when there are nested gratuitous parens. if (((a) == b)){
Fabulous. Pretty much all of the errors in the demo are now quick fixes. http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b63fee8ab0e5e85af3f29f222b3ead4a42151a62 New quick fix + tests Had to make a few alterations to our fix all support for this. 1) Allow quick fix all to provide multiple text edits per annotation 2) Sort all the edits by start range (otherwise the multiple edits can stomp on each other) Updating the wiki now. |