Standardize and share Dash linting rules #1578
Conversation
dash-renderer/.eslintrc
Outdated
| "rules": { | ||
| "no-constant-condition": 0, | ||
| "no-prototype-builtins": 0, | ||
| "no-unused-vars": 0, |
There was a problem hiding this comment.
The renderer's configuration has some peculiarities that shouldn't be generalized to all projects. Also no-unused-vars just acknowledges the current state of the JavaScript code in this project not adhering to this rule.. there's a lot of dangling unused variables in JS files at the moment 😢
There was a problem hiding this comment.
Hmm OK, would be nice to tighten up no-unused-vars at some point, and see whether the others actually have a legitimate reason to be violated. Care to make an issue to follow up on this later?
There was a problem hiding this comment.
I see, I missed a detail here: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars.md
// note you must disable the base rule as it can report incorrect errors
"no-unused-vars": "off",
Also used the occasion to fix the .eslintrc config so that it is loaded correctly by VSCode when it references a tsconfig.
Marc-Andre-Rivet
changed the title
| @@ -0,0 +1,3 @@ | |||
| # @plotly/prettier-config-dash | |||
|
|
|||
| Shareable Prettier configuration for Dash projects. No newline at end of file | |||
There was a problem hiding this comment.
Not a big deal, but is it important that this is a separate package? Feels like it would be lighter to use if this were in the same package as the eslint config, and they're intended to be used in tandem.
There was a problem hiding this comment.
The prettier configuration could be added to the eslint-config-... one but it would need to be the default module export (so including "@plotly/eslint-config-dash" in eslint would be invalid and would return the Prettier configuration). It couldn't be the other way around because eslint configs must be published under @<scope>/eslint-config-<package> or eslint-config-<package>. All-in-all agree it would be nice if it could be simpler but given the limitations I think it's best to just keep them separate.
There was a problem hiding this comment.
OK. I was hoping we could do something like "prettier": "@plotly/eslint-config-dash/prettier" but even if that works it does look a bit odd to have prettier explicitly nested under eslint.
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
LGTM, just a couple of small comments. 💃
Addition to #1576 that allows standardizing
eslintandprettierconfigurations across Dash projects.@plotly/eslint-config-dashthat exposes two base linting configurations for JavaScript and TypeScript.@plotly/prettier-config-dashthat exposes a basic Prettier configurationThese proposed configurations are based on the intersection of the Core's projects current configurations.
Both
@plotly/eslint-config-dashand@plotly/prettier-config-dashwill need to be published to NPM to be useable and to close this PR.Percy visual tests will fail because of the un-merged upstream change from py37 to py39.