-
Notifications
You must be signed in to change notification settings - Fork 6
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
GH-14: Use the latest official version of grunt-eslint. (resolves #14) #15
Conversation
Removing the need to use the fluid-grunt-eslint fork. However this does include a fork of the stylish formatter to output the number of linted files when all pass. In addition to removing the forked grunt task, it allows for tracking newer versions of eslint; which provides access to linting of modern JS syntax.
package.json
Outdated
}, | ||
"dependencies": { | ||
"@textlint/markdown-to-ast": "6.2.5", | ||
"chalk": "4.1.0", | ||
"eslint": "7.9.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already out of date, 7.10.0
is out now.
Sorry for the delay in responding. This is largely fine, I did want to put a little more thought into the patch strategy for the formatter, although I guess we can file that away and just update to the latest version for now. |
- Update dependencies - add TODO: comments with links to issues.
@the-t-in-rtf ready for more review. |
So, @amb26 and I have discovered some fairly serious issues with both master and the last released version (1.0.8). We need to address these before we can merge this pull. In short:
I will confirm shortly, but I suspect that upgrading to ESLint 7 as part of the migration brought in changes that result in the change in behaviour. My hope is that we can create a 1.0.9 release with the last safe version of ESLint 6.x, and deprecate 1.0.8 in favour of 1.0.9. We then need to find a safe way to move to ESLint 7, which I will start as a separate pull. As part of this pull, I plan to add a CI task to lint Infusion with the plugin as it will behave when released. This should hopefully help us work through the issues involved. If we can find a configuration workaround for all issues, we would merge this pull and create a 1.0.10 or 1.1.0 release that includes both. If ESLint 7 results in a lot of new linting errors that we want to keep, then we would likely create a 2.0.0 release with this work, just to reflect the nontrivial work a user of the package would have to put in to upgrade. |
I have created a new pull to add linting Infusion as a CI step, which demonstrates the indenting issues in this CI build. I suspect the |
OK, it took a while to separate the real from phantom issues. In short, migrating fluid-grunt-lint-all from fluid-grunt-eslint to grunt-eslint does indeed highlight a significant number of lint errors when run against infusion, namely violations of the We looked into the possibility of writing a CI job that lints infusion with the tasks in a given branch. That approach did not pan out. I could not get I also tried various strategies for running the checks from the root of the fluid-grunt-lint-all repo against a checked out copy of infusion in a subdirectory, which correctly picks up the tasks we want to test. However, that picks up the eslintrc and eslint-config-fluid from the repo rather than those used in infusion, and would report errors as a result of not having picked up the correction configuration for the ESLint In talking with @amb26, it sounds like the most straightforward way to proceed is to ask contributors to submit a parallel pull against Infusion for each pull against fluid-grunt-lint-all (or eslint-config-fluid). The two pulls would be worked on in parallel, adjusting rules in the fluid-grunt-lint-all (or eslint-config-fluid) pull, fixing legitimate linting errors in a pull against infusion.That would accomplish a few key things:
I realise this makes it more difficult to contribute to this package, but the alternative seems like it would make it simpler to review rules and configuration changes in this package, at the cost of piling up mountains of work for whomever tries to update Infusion's dependencies next. |
@the-t-in-rtf I've created a parallel Infusion PR: fluid-project/infusion#1013 |
Removing the need to use the fluid-grunt-eslint fork. However this does include a fork of the stylish formatter to output the number of linted files when all pass.
In addition to removing the forked grunt task, it allows for tracking newer versions of eslint; which provides access to linting of modern JS syntax.
Resolves #14
This should be updated to use the latest eslint-grunt-config after fluid-project/eslint-config-fluid#8 has been merged to address FLUID-6548.
Parallel Infusion PR: fluid-project/infusion#1013