Community
Participate
Working Groups
This is just an idea. The point is this. When you have a warning, sometimes you just want to ignore this particular one (like @callback) or change the settings for everyone. Let's explore what makes sense in the context of a quickfix.
Here is another example: function mouseDown(event) { //TODO - not used } This function has 3 warnings associated with it. I might decide that it is ok to shadow "event" everywhere in my code. How can I tell Orion to that shadowing this variable is ok? Likely there is an ESLint syntax. It would be cool to be able to do this in a quickfix ... maybe? Discussion needed.
(In reply to Steve Northover from comment #2) > Here is another example: > > function mouseDown(event) { > //TODO - not used > } > > This function has 3 warnings associated with it. I might decide that it is > ok to shadow "event" everywhere in my code. How can I tell Orion to that > shadowing this variable is ok? Likely there is an ESLint syntax. It would > be cool to be able to do this in a quickfix ... maybe? Discussion needed. I envision a few fixes for this: 1. "ignore in-file" - which would add the /*eslint-disable <rule id>*/ to the file 2. "ignore all problems like this in workspace" - sets the prefs to be ignore 3. "ignore all problems like this in this project" - sets the prefs in the eslintrc file
From a discussion with Mike, he suggests: /*eslint-disable <rule-id>*/ but I only want to disable the rule for shadowing of "event". I know you can turn of linting for a section of code but this gets messy when the section is one variable.
Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see: https://dev.eclipse.org/mhonarc/lists/orion-dev/msg04002.html
Reopening
We need to address this one. Sometimes shadowing is ok in some code but not in general because I have looked at the code and decided that it is ok here, but in general, I want to be warned when I do it. I need something like @callback pragma.
Created attachment 266499 [details] The hover
Here is an example. For some reason, I don't want to fix this error. 1) I might want to go to the settings and turn it off 2) I might want to ignore this particular instance Others? The thing is that I want to deal with it in place and move on. I don't want to keep seeing it.
Another options is to insert eslint directives at the top of the file as part of the quickfix. /*eslint-env node */ /*eslint no-else-return: "none"*/ var test = require('./some_module.js'); function fred () { if (FRED) { return 12; } else { return 13; } } BTW, this currently fails but should work.
It is super painful to decide you don't want an eslint error and then try to find it in the UI. Where is the "no-else-return" on the settings page?
(In reply to Steve Northover from comment #11) > It is super painful to decide you don't want an eslint error and then try to > find it in the UI. Where is the "no-else-return" on the settings page? I started a discussion previously of adding the rule names to the settings page, but we agreed that it was better to provide tooling to fix it for the user. no-else-return "Unnecessary else after return" https://wiki.eclipse.org/Orion/ESLint
The idea: I am right there in the code and I should be able to fix the code, ignore the warning for the file or ignore the warning globally without losing context.
I like these: 0. "ingore this-instance" - insert @callback or pragma around the code 1. "ignore in-file" - which would add the /*eslint-disable <rule id>*/ to the file 2. "ignore all problems like this in workspace" - sets the prefs to be ignore 3. "ignore all problems like this in this project" - sets the prefs in the eslintrc file 2) and 3) depend on whether an .eslintrc file is being used. Mike, SSQ?
(In reply to Steve Northover from comment #14) > I like these: > > 0. "ingore this-instance" - insert @callback or pragma around the code > 1. "ignore in-file" - which would add the /*eslint-disable <rule id>*/ to > the file > 2. "ignore all problems like this in workspace" - sets the prefs to be ignore > 3. "ignore all problems like this in this project" - sets the prefs in the > eslintrc file > > 2) and 3) depend on whether an .eslintrc file is being used. > > Mike, SSQ? That would be a lot of quickfixes with long names, tooltips will be quite large for every single linting problem. Might have to look at some new UI bits.
(In reply to Steve Northover from comment #14) > I like these: > > 0. "ingore this-instance" - insert @callback or pragma around the code > 1. "ignore in-file" - which would add the /*eslint-disable <rule id>*/ to > the file > 2. "ignore all problems like this in workspace" - sets the prefs to be ignore > 3. "ignore all problems like this in this project" - sets the prefs in the > eslintrc file > > 2) and 3) depend on whether an .eslintrc file is being used. > > Mike, SSQ? Those all make sense to me, but I have the same concern as Curtis - thats a lot of fixes to show for every single problem. My fear is that the useful fixes will be lost in the noise of the ignore fixes - unless we could have some other kind of UI for the ignore fix (like a drop down or something to choose the way to ignore, I dunno)
Are these the cases? Fix ( ) here, ( ) in file, ( ) in project, ( ) in workspace Ignore ( ) here, ( ) in file, ( ) in project, ( ) in workspace Fixing all the code everywhere would be incredible (possibly). This is like refactoring where you get a UI to review the changes, with things selected etc.
(In reply to Steve Northover from comment #17) > Are these the cases? > > Fix ( ) here, ( ) in file, ( ) in project, ( ) in workspace > Ignore ( ) here, ( ) in file, ( ) in project, ( ) in workspace That looks correct to me. > > Fixing all the code everywhere would be incredible (possibly). This is like > refactoring where you get a UI to review the changes, with things selected > etc. That would be very cool. There are certain classes of problems we could easily make a "fix in project" scheme for (because we know 100% the code change is non-breaking, like the "semi" rule)
Created attachment 266578 [details] Adds Ignore in file quickfix This patch adds a quick fix that will add an eslint-disable property to your file to disable whatever lint warning you are hovering on. It sets up the framework for more such quickfixes by passing along the rule ID in the annotation data. Next steps: 1) Finish the logic to handle updating eslint-enable, working around eslint-env directives, etc. 2) Add a test suite for the quick fix 3) Create a quick fix that will update project settings 4) Look at a custom UI in the quick fix rendering
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=660ee2d75baf017163d4d784c6dc3e96e61d20ac I pushed in the framework bits, quickfix implementation and the test suite to master. The quick fix command is hidden behind a dark launch flag. Set localStorage.ignoreQuickfix = true, then have the javascript tooling plugin restart.
Perfect. Will try it out tomorrow.
There is a bug that 'Ignore in file' scrolls to the top of the file, causing you to lose context. Please fix this.
The quickfix should provide a link to the global setting or eslint.rc, depending on which is being used. IDEA: The simplest UI could be a 3 buttons and a link: Fix, Fix All, Ignore All, Settings ... We forget about telling the guy what we are going to do (ie. 'Update Operator') on the text of the button (or we could tell him elsewhere).
(In reply to Steve Northover from comment #22) > There is a bug that 'Ignore in file' scrolls to the top of the file, causing > you to lose context. Please fix this. Whilte I could see some user wanting to know what was changed in the file, I agree that we shouldn't interrupt the user's workflow. I will use some of the logic we use for quick fix all to keep the annotation location visible. (In reply to Steve Northover from comment #23) > Fix, Fix All, Ignore All, Settings ... > > We forget about telling the guy what we are going to do (ie. 'Update > Operator') on the text of the button (or we could tell him elsewhere). While having a simpler UI would be great, I would have difficulty trusting a tool to Fix All problems when I have no idea what kind of change it is going to make (remember some quick fixes delete code). I think we should keep the existing quick fix text, possibly reopening the discussion on styling them as links rather than buttons. Then have some separate UI changing ignore settings.
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=e50c54f29dc1d3ae96776d92758a17cda0968115 Fixed issues with the eslint-enable edit and added tests (In reply to Steve Northover from comment #22) > There is a bug that 'Ignore in file' scrolls to the top of the file, causing > you to lose context. Please fix this. This is more difficult than expected, turning off the show selection flag is not enough. After the edits are done, all the previous offsets are wrong. If I don't find another workaround I will have to add the support in the setText function.
(In reply to Curtis Windatt from comment #25) > (In reply to Steve Northover from comment #22) > > There is a bug that 'Ignore in file' scrolls to the top of the file, causing > > you to lose context. Please fix this. > > This is more difficult than expected, turning off the show selection flag is > not enough. After the edits are done, all the previous offsets are wrong. > If I don't find another workaround I will have to add the support in the > setText function. http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=97d20d1baf4d821573766a3eb3e8ff5c665e5fb8 Ok, found a workaround, I include an additional no-op edit that will set the selection to where the hovered annotation was.
Created attachment 266688 [details] Example of using links After experimenting with a few UI controls, I still think links are the way to go. I'm thinking a line of text "Ignore in:" followed by links with better spacing "File" "Workspace" ".eslintrc".
Hmm ... looks confusing. Please mock up 3 buttons: [Fix][Fix all][Ignore all] The text that describes the fix can be a hover. This will catch all the cases we have today and be clear about will happen when I press the button. If this looks ok, we can revisit doing better and capturing all the cases in a better UI.
(In reply to Steve Northover from comment #28) > Hmm ... looks confusing. Please mock up 3 buttons: > > [Fix][Fix all][Ignore all] > > The text that describes the fix can be a hover. This will catch all the > cases we have today and be clear about will happen when I press the button. > If this looks ok, we can revisit doing better and capturing all the cases in > a better UI. Would [Ignore all] set the disable directive in the file or modify your workspace settings or modify eslintrc? For the mockup it will just use eslint-disable as I have that implemented.
Created attachment 266707 [details] JDT fixes I think we should follow JDT's lead here. The attached screen shot shows all the fixes for a given problem (in JDT), and includes a link to changes the prefs. The presentation is clear, concise and the user understands what each link will do with a glance. In our case though, we would also want a fix to "ignore in file".
The Eclipse UI looks nice. These are all separate and different fixes for the same problem. Would we unroll the (x) check box? What does the UI look like with links for the particular case? Three links? We need to provide this functionality: [Fix][Fix all][Ignore all] Are these the links: "Update operator" "Update operator in file" "Ignore in file" Are we making sentences out of "Update operator"? Where does that string come from.
Created attachment 266708 [details] Links on separate lines
Created attachment 266709 [details] 3 buttons on lone line
Created attachment 266710 [details] Comparison of different UIs
Created attachment 266716 [details] Matching Eclipse style 3 links, indents the fix all when available just like Eclipse. This is fully functional, not just a mock up. Changing the link color and underline is a little harder because the page theme overrides link (a) elements.
I'm guessing that if I hover with the mouse, I see the links? Without the hover, it looks like the text is mis-aligned/badly formatted. If we are giving links, then these are like Windows 7 "Command Buttons". They contain sentences and icons. This is essentially what Eclipse is doing. This sounds like work because you need an ignore sentence and it should not be constructed from the "fix action" sentence fragment.
Created attachment 266833 [details] Use an expanding div
Created attachment 266834 [details] Screenshot of expanding div
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b443c3506fb296c184dda3a403485b21e607040b Removed dark launch flag Fixed command to only show when ruleID is set Disable fix will always show at bottom of quick fix list Closing as FIXED. I will open new bugs for future enhancements.
