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

Update to use ESLint without formatting rules and plugin with them. #18

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

duhrer
Copy link
Collaborator

@duhrer duhrer commented Nov 4, 2023

See #17 for details.

@duhrer
Copy link
Collaborator Author

duhrer commented Nov 4, 2023

I have cut [email protected] to test these changes downstream in fluid-lint-all. This pull should be reviewed concurrently with the upcoming pull against fluid-lint-all and merged just before that one so we can cut a full release.

index.js Outdated
Comment on lines 18 to 19
// foo

Copy link
Member

Choose a reason for hiding this comment

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

Was this change intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Must have been a wrong window kind of thing, thanks for catching it. It's gone now.

@duhrer
Copy link
Collaborator Author

duhrer commented Nov 9, 2023

Just a note here, I noticed that the current version was "backward facing", i.e. it matches the previous full release, so dev releases of this work were showing up before the last full release in npm info eslint-config-fluid versions (and presumably other places).

I updated this pull to use the next patch release. I can bump to the next minor release if preferred.

Copy link
Contributor

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

@duhrer Looks great. I did have one question related to the Grunt dependencies.

Comment on lines 33 to 37
"devDependencies": {
"grunt": "1.5.3",
"grunt-eslint": "24.0.0",
"grunt": "1.6.1",
"grunt-eslint": "24.3.0",
"grunt-jsonlint": "2.1.3"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not strictly related to this PR but I wonder if it might be worth removing grunt etc if it's only being used for linting this package and eslint is now a dependency? If we were to do so we could change the lint script to eslint with an appropriate glob rule to target all the JS and JSON files in this package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's good to handle in another issue, as this small change is needed to allow any projects to work with recent versions of ESLint.

I wrote that up as #19, and left a comment there for you. Happy to work on that once we have a chance to talk about what we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made a first pass at #19 in a separate pull (#20).

@amb26 amb26 merged commit 44cf2b3 into fluid-project:main Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants