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

Enable processing of further plugins #39

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

stephtr
Copy link

@stephtr stephtr commented Mar 22, 2020

This is an approach to fix #38, to enable using this plugin in combination with eslint-plugin-prettier.
This change modifies JSON code to be valid JS (by embedding it into a JSON.stringify function, which won't get executed) and furthermore enables autofixing.

test/unit.test.js Outdated Show resolved Hide resolved
@AdrieanKhisbe
Copy link
Collaborator

damn, I wasn't supposed to push on your branch without asking 🤦‍♂
sorry about that @stephtr :/

@codecov-io
Copy link

codecov-io commented Mar 23, 2020

Codecov Report

Merging #39 into master will decrease coverage by 1.35%.
The diff coverage is 95.83%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #39      +/-   ##
===========================================
- Coverage   100.00%   98.64%   -1.36%     
===========================================
  Files            1        1              
  Lines           51       74      +23     
===========================================
+ Hits            51       73      +22     
- Misses           0        1       +1     
Impacted Files Coverage Δ
src/index.js 98.64% <95.83%> (-1.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bbe2bc...72e2bdd. Read the comment docs.

Copy link
Collaborator

@AdrieanKhisbe AdrieanKhisbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add a look on the Pr, and this ends up in two question bellow and some edits (that were supposed to be on a branch on that repo, and not yours 😏).

I found the reason for the fail:

  • It's due to the fact that eslint-plugin-json dont rely on preprocessor output, and eslint: vscode-json-languageservice does the job under the hood, and we convert errors in postprocessor.
  • To fix it, in the mapValues you added, I added a shortcircuit for json/error not to be affected in the column and endColumn modifications.

So right now, this version does not seems to break anything, but I have no way to tell it would be working.
We should add a prettier config in the tests and make it run to ensure other processor can process the the json files

src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@stephtr
Copy link
Author

stephtr commented Mar 24, 2020

that were supposed to be on a branch on that repo, and not yours

This way it's also perfectly fine!

Ah thanks, I forgot that the rules of this plugin don't use the output of the preprocessor. That affects the failing test and the error mapping.

I changed the eol regex from /([\n\r]{0,2})?$/ to /([\n\r]*)$/. I think it makes more sense to convert {}\n\n to JSON.stringify({})\n\n than to JSON.stringify({}\n)\n.

I'll add a test case together with Prettier soon.

@AdrieanKhisbe
Copy link
Collaborator

@stephtr
Cool, I'll wait until you add the prettier integration test then. :)

(by the way, prettier might want you to add a semi column to the preprocessorTemplate, and maybe a trailing newline)

src/index.js Outdated
Comment on lines 164 to 168
fix:
error.fix &&
_.assign(error.fix, {
range: error.fix.range.map(location => location - prefixLength)
})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This by the way didn't work. For error.fix=undefined _.assign resulted in a property fix: undefined, which isn't compatible with eslint.

@stephtr
Copy link
Author

stephtr commented Mar 24, 2020

(by the way, prettier might want you to add a semi column to the preprocessorTemplate, and maybe a trailing newline)

Prettier recognizes the file type via the file extension. In case of JSON files it doesn't add semicolons or singleQuotes (neither does eslint). We need the preprocessorTemplate stuff only to get through eslint, fortunately it doesn't affect Prettier, even though it's illegal JSON. That's by the way a reason why I'm thinking whether we should "hide" this PR behind a config flag, then there also wouldn't be a breaking change.

Concerning the trailing newline: If the original file contains a trailing newline, it gets appended to the template. If not and Prettier adds it, the fix gets applied to the original file, which is exactly the behavior we want.

@AdrieanKhisbe AdrieanKhisbe mentioned this pull request May 12, 2020
3 tasks
@@ -104,6 +105,30 @@ const errorSignature = err =>

const getErrorCode = _.pipe(_.get('ruleId'), _.split('/'), _.last);

const preprocessorPlaceholder = '___';
const preprocessorTemplate = `JSON.stringify(${preprocessorPlaceholder})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is JSON.stringify even necessary? Just wrapping the JSON is parentheses would make for valid JS: ({"foo":1})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as soon as additional eslint rules kick in (for example no-unused-expressions) or prettier tries to parse the file with babel, things start to fall apart. One could work around those issues by fine-tuning the eslintrc file, but just adding JSON.stringify simplifies things in practice a lot.

@dentuzhik
Copy link

Hey guys, is this any soon to be merged?
We tried to integrate to our homebacked config, but because of this issue, we had to turn it off.

@azeemba
Copy link
Owner

azeemba commented Dec 9, 2020

Hey folks, I have been trying to look through the change and understand its potential impact. I have generally been nervous about making significant changes that can potentially break existing users. Honestly, the eslint ecosystem has changed a lot and I have not kept up with it which makes me extra nervous.

That being said, I do understand that as the ecosystem changes, a different solution might be more valuable. Recently someone shared this alternative project that seems to provide a JSON parser for ESLint with built in support for auto-fixing some issues: https://github.com/zeitport/eslint-parser-json. I think it might be worth checking if this solution fits your use case better.

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

Successfully merging this pull request may close these issues.

Combination with eslint-plugin-prettier
6 participants