pvh
commented
3 years ago Okay, I've now cleaned up all the remaining easy stuff. There is a little bit of test weirdness that results in a few disabled rules for those test files but the actual project code is linting clean. I just saw in the Travis build above that some version of Edge doesn't like something in one of the tests but it's a bit late to start debugging Microsoft browsers over here, sorry -- hope it's either a CI glitch or else really obvious.
Other than that, the last bit that really remains here is fixing the one real bug, which is the way we're returning objects from constructors. This will likely result in an API change and so I didn't want to include it in this branch.
Let me know if you have questions / see any problems and I'll weigh in when I arise tomorrow.
ept
This is a draft patch enabling
eslintfor Automerge. This first patch leaves a number of known bugs and problems found by eslint in place in the interest of first making some decisions about whether we want to move forward with this approach or not.In almost every case I've either disabled an eslint rule project-wide or on the particular line where the complaint from the linter occurred, but in a few cases I decided the fixes were uncontroversial enough just to tidy up on the spot.
In particular, the most important rule I think we still ought to enable is
no-unused-vars. That will require making a decision about tests which have loads of unused variables that are kept around for visual consistency. I think it would be a fine compromise to enable no-unused-vars overall but consider leaving it disabled for tests (at least for the time being.)I'll add some comments on the patch below. If we can reach an agreeable set of eslint rules and land them we can always tune them later, either adding new rules to improve consistency (many of the rules I disabled from the base were only triggered by one or two cases) or to better suit Martin's preferred style.