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

lint: add eslint-plugin-dprint #3212

Merged
merged 2 commits into from
Aug 1, 2021
Merged

lint: add eslint-plugin-dprint #3212

merged 2 commits into from
Aug 1, 2021

Conversation

quisquous
Copy link
Owner

@quisquous quisquous commented Jun 28, 2021

Here's an alternative patch to #3211 trying out dprint. This is an eslint plugin that runs dprint, but does a lot of things that conflict with other eslint rules. I've adjusted indent to match what dprint does (for consistency), as well as disabled a few rules that dprint handles (nearly the same) as eslint so that they don't fight.

eslint-plugin-dprint runs twice as fast as prettier-eslint, but still 50% slower than eslint on its own for me.

$ time npm run lintfix

> cactbot@ lintfix C:\Users\enne\cactbot
> eslint --max-warnings=0 --fix ./


real    1m37.033s
user    0m0.015s
sys     0m0.106s

Sadly, dprint on its own takes literally 8 seconds for a full format, but we still have a lot of eslint rules and valid custom eslint rules and so it's not really feasible to only run dprint.

I also ran into a few issues:

  • It rearranged some eslint-disable comments in dfa7dd1 that I manually adjusted to not get broken.
  • CombatantJobSearch took 20 minutes to format, and so I disabled it (?!) once formatted, CombatantJobSearch was fast again but formatting a large packed array apparently is just bad
  • There's some weird errors where dprint really wants a linebreak in some raidemulator files (but it's not clear how to satisfy it, and it will not auto fix, see e9841e7)

Copy link
Contributor

@panicstevenson panicstevenson left a comment

Choose a reason for hiding this comment

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

I think I prefer this method since these will be shoved directly into ESLint, which means that dprint will be at the VSCode level.

As you wrote, I don't think there's a way to avoid ESLint (not enough rules in dprint, custom rules, etc.), so I think we should lean into its ability to be shifted left to IDE integration (in which, I think most cactbot developers already have), or, at worst, be not an additional long-winded thing after ESLint.

One thing to think about with this method: will dprint let you use long line widths? I know Prettier will format things around the line width specified, so making the value really high will turn things we've broken up into multiple lines into one gigantic line, but does this do the same? If not, maybe it's worth setting the lineWidth really high for a first pass since I think that's where most of the churn is coming from.

.eslintrc.cjs Show resolved Hide resolved
@quisquous
Copy link
Owner Author

I think I prefer this method since these will be shoved directly into ESLint, which means that dprint will be at the VSCode level.

Yeah, I agree with that feeling too. It's nice to have the warnings there when things look weird. (Hopefully it won't be too much.)

One thing to think about with this method: will dprint let you use long line widths? I know Prettier will format things around the line width specified, so making the value really high will turn things we've broken up into multiple lines into one gigantic line, but does this do the same? If not, maybe it's worth setting the lineWidth really high for a first pass since I think that's where most of the churn is coming from.

I had the same thought, and the unfortunate answer is no. If you've got a giant array, dprint will happily stick that all on one giant line if it fits just like prettier does.

I think a lot of the churn on lineWidth comes from really long strings, but I don't see any option that says "don't bother breaking if it's not going to fit anyway".

@quisquous quisquous marked this pull request as ready for review July 1, 2021 03:40
@quisquous quisquous mentioned this pull request Jul 1, 2021
@quisquous
Copy link
Owner Author

I'm not going to rebase this a million times since it touches every file. I'll leave this up for folks to look at and play with, but if we go this route, I'll land the "change indent to 2", "preliminary fixes", "add dprint + format" steps all separately.

dprint.json Outdated
@@ -0,0 +1,34 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you remember why it was necessary to have a dprint config file outside of the eslint config definition? Are we using dprint as a standalone ever (for markdown/json?)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Uhhhhh, I think maybe I forgot to remove it when I changed to the eslint version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, okay; sounds fair! I just tested this myself and didn't need it, so I was curious. I couldn't find a way around the line-width unfortunately, I don't think there's a way given how it operates. So, overall this LGTM!

quisquous added a commit that referenced this pull request Aug 1, 2021
This is pre-refactoring prior to landing #3212.
quisquous added a commit that referenced this pull request Aug 1, 2021
This is pre-refactoring prior to landing #3212.
github-actions bot pushed a commit that referenced this pull request Aug 1, 2021
This is pre-refactoring prior to landing #3212. e380ab3
github-actions bot pushed a commit that referenced this pull request Aug 1, 2021
This is pre-refactoring prior to landing #3212. e380ab3
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.

3 participants