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

When fixable rules are overlapping, It takes multiple format/fix commands to reach the same result as cli xo --fix #113

Closed
gutenye opened this issue May 26, 2022 · 7 comments

Comments

@gutenye
Copy link

gutenye commented May 26, 2022

The .xo-config.cjs

Using eslint-plugin-simple-import-sort

// yarn add --dev eslint-config-simple-import-sort
module.exports = {
  prettier: true,
  plugins: ['simple-import-sort'],
  rules: {		
    'simple-import-sort/imports': 'error'
  }
}

This code

import {b,a} from 'file'

is formatted into below using VSCode Format Document with - XO

import {a,b} from 'file'

is formatted into below using Cmd xo --fix

import { a, b } from 'file'

The result is different.

I guess the Cmd runs eslint fix -> prettier in order, but the VSCode runs eslint fix only.

@spence-s
Copy link
Collaborator

spence-s commented May 26, 2022

The extensions does not run things any differently than the CLI. There is no prettier or eslint step, they are both ran at the same time by eslint under the hood in xo.

  1. Try setting the "xo.runtime" setting in VSCode so you are using the same nodejs binary for both the cli and the extension. Otherwise, by default xo in vscode will use the version of nodejs bundled inside of vscode which can cause different results from the cli. (run which node in your shell to get the path to the node binary you are currently using)

  2. Make sure you have all other extensions disabled and aren't seeing changes from another lib.

Please let me know if this fixes the issue.

@gutenye
Copy link
Author

gutenye commented May 27, 2022

Setting the xo.runtime didn't help. I output the edits and result and find out the problem

"edits": {
   "[0,0,0,21]-simple-import-sort/imports": {
      "label": "Fix this simple-import-sort/imports problem",
      "documentVersion": 13,
      "ruleId": "simple-import-sort/imports",
      "edit": {
        "range": [ 0, 21 ],
        "text": "import {a,b} from 'file'"
      }
    },
    "[0,8,0,11]-prettier/prettier": {
      "label": "Fix this prettier/prettier problem",
      "documentVersion": 13,
      "ruleId": "prettier/prettier",
      "edit": {
        "range": [ 8, 11],
        "text": " b, a "
      }
    },
"result": {
    "documentVersion": 13,
    "edits": [
      {
        "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 0, "character": 21 }
        },
        "newText": "import {a,b} from 'file'"
      }
    ]
  }

The VSCode runs the fix at the same time and output an overlap free result. I guess the xo cmd runs the fix in sequence.

@spence-s
Copy link
Collaborator

spence-s commented May 27, 2022

@gutenye - thank you so much for digging into this problem. I will look more deeply into this and see if I need to strip out some of the old overlap logic to keep things more consistent with the cli.

@spence-s
Copy link
Collaborator

So after taking a look I realized that this is just a known issue. There is actually not a core problem, you just have to run format 2x to get the same results as the CLI for some overlapping rules. This is something I am interested in fixing long term, but it's not a very high priority since the work around is just executing format 2x.

This is a duplicate of #42

Unfortunately, solving this issue is not as trivial as "run x before y" - we will need to recreate the same logic that eslint already uses to handle cases where a single line has overlapping fixes and I believe it's complicated.

I will keep this open until I have time to fix, but this will likely remain a low priority since its a very minor inconvenience and the extension still gets to the same final result after a couple of format commands.

@spence-s spence-s changed the title Different result runs with cmd xo --fix It takes multiple format/fix commands to reach the same result as cli xo --fix May 28, 2022
@spence-s spence-s changed the title It takes multiple format/fix commands to reach the same result as cli xo --fix When fixable rules are overlapping, It takes multiple format/fix commands to reach the same result as cli xo --fix May 28, 2022
@gutenye
Copy link
Author

gutenye commented Jun 1, 2022

Save two or more times is not an option for me, every time I save a file, I have to take extra time to check if there's still eslint error? If so, save it again, if not, skip it. It's slow and not developer-friendly.

Will this solution be easier? Add a editor.codeActionsOnSave option.

The Eslint VSCode Exension has this option

 "editor.codeActionsOnSave": {
      "source.fixAll.eslint": true
  },
  "editor.formatOnSave": true,
  "editor.defaultFormatter": "esbenp.prettier-vscode"
}

The VSCode will run editor.codeActionsOnSave (eslint fix) first, then editor.formatOnSave (prettier), problem solved.

Unfortunately, solving this issue is not as trivial as "run x before y" - we will need to recreate the same logic that eslint already uses to handle cases where a single line has overlapping fixes and I believe it's complicated.

Another idea, can we change

report = await xo.lintText(contents, options);

to

const result = await xo.lintText(contents, { ...options, fix: true });
result contains the overlay-free text.

when we perform a fix. This one does not need to recreate the same logic.

@spence-s
Copy link
Collaborator

spence-s commented Jun 1, 2022

Save two or more times is not an option for me, every time I save a file, I have to take extra time to check if there's still eslint error? If so, save it again, if not, skip it. It's slow and not developer-friendly.

eslint suffers from this too, but is admittedly a little better than this xo extension --- try formatting big unformatted files with eslint, xo + unicorn plugins and prettier. -- You will have to save multiple times. Also, it doesn't happen very often in this extension at all, it is not nearly as inconvenient as you are making it out to be.

Will this solution be easier? Add a editor.codeActionsOnSave option.

Maybe - this is on the road map. I have recently started supporting code actions, but I haven't done the code actions on save. Feel free to take a look at the LSP spec and implement this. I don't really think this will be any simpler, it will still have to deal with the overlapping problem.

Another idea, can we change

Feel free to test this out! I will play with this idea. My suspicion is that this will not work well at all because it will cause the squiggly error underlines in vscode to be in the wrong places, or results won't return all the correct information for the original errors, so it will be missing errors and flagging the wrong places.
If this were to work, and be this simple, then that would be great!...
However, I am not optimistic until I see and test it.

Note that every lint error already has the "fixed" result in it, if it is fixable, even without the "fix" option. The likely only way to really fix this is to manually implement logic in getDocumentFixes method.

And please trust me that I have attempted to fix this problem on numerous occasions because I do know it is annoying when it happens, but I haven't found an easy way yet.

I do accept PRs, although I have only got 1 I think since taking over maintenance of this extension.

@spence-s
Copy link
Collaborator

Was able to get it working in the latest release so that this should no longer occur, thanks for the suggestions

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

No branches or pull requests

2 participants