geomoose / gm3

GeoMoose 3.0 Development. Please submit pull requests to the 'main' branch.
https://www.geomoose.org
MIT License
58 stars 59 forks source link

Update ESLint and Switch to Prettier Rules #763

Closed theduckylittle closed 1 year ago

theduckylittle commented 1 year ago

Super big commit but it does :sparkles: magic.

klassenjs commented 1 year ago

The overall style looks reasonable to me.

Is this still checking for common coding errors like if (a = b), unused variables, etc. type stuff? (Edit: YES)

Is there a magic button to fix formatting issues instead of just complain about them? (Edit: YES eslint --fix)

klassenjs commented 1 year ago

Not a new bug, but while testing the new rules I found that src/index.js is escaping the watchful eye of eslint.

klassenjs commented 1 year ago

The various top-level project config files also aren't currently being linted (.eslintrc.js, webpack.config.js, jest.config.js, ...)

And I'm not sure we'd want these linted to the same standard, but examples is also ignored.

klassenjs commented 1 year ago

My main concern about this PR is impact on downstream branches. As this changes just about every file it would create a mess of merge conflicts with any changes that might have been made downstream. I don't know how much a problem this is in reality. I also don't know how much of this we should take on as our problem. It would primarily impact people who have customized the core GeoMoose library which is outside of what the project claims for API stability (edits in examples aren't impacted). But those who have made core library changes are also likely some of our most invested users. That said, I don't think we should forever avoid harmonizing and improving coding standards because it might cause an issue.

My current thinking is it might be helpful to break this PR into two commits. (1) Small commit that just applies the configuration changes. (2) A commit that applies the new style across the codebase. My thinking is this would let a downstream branch cherry-pick (1) and then run eslint --fix before trying to merge with main and hopefully that would limit the merge conflicts???

I could also be easily convinced that I'm over thinking this.

theduckylittle commented 1 year ago

I feel as if there is not enough weight in outside branches in order to be concerned.

theduckylittle commented 1 year ago

If this PR is accepted then I think we setup a follow up issue to fix:

klassenjs commented 1 year ago

I'm +1 on merge when ready.

theduckylittle commented 1 year ago

Deprecating in favor of #779