Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

SJRK-447: Switch to fluid-lint-all #107

Merged
merged 3 commits into from
Jan 25, 2021
Merged

Conversation

jobara
Copy link
Member

@jobara jobara commented Jan 20, 2021

https://issues.fluidproject.org/browse/SJRK-447

This PR is based off of work in #105

@jobara
Copy link
Member Author

jobara commented Jan 21, 2021

@cindyli and @BlueSlug, this PR is ready for review.

@jobara jobara requested a review from BlueSlug January 21, 2021 14:54
@jobara
Copy link
Member Author

jobara commented Jan 21, 2021

Actually there may be a new release of fluid-lint-all coming soon that will make the configuration a bit easier by automatically excluding the images and etc. We should check the state of that before merging this. If it's available or will be soon, we can hold off. Otherwise we can make improvements later.

@jobara
Copy link
Member Author

jobara commented Jan 25, 2021

@cindyli and @BlueSlug I've updated to the latest version of fluid-lint-all (v1.0.2). This is ready for review.

Comment on lines 44 to 45
"eslint-config-fluid": "2.0.0",
"eslint-plugin-jsdoc": "30.7.8",
Copy link
Member

Choose a reason for hiding this comment

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

Since eslint-config-fluid and eslint-plugin-jsdoc have already been dependencies of fluid-lint-all, is it necessary to include them again as dependencies of this repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

We recommend for "eslint-config-fluid" that you explicitly also depend on "eslint-plugin-jsdoc" if you use it. The issue is that there are problems with packaging an eslint plugin with the shareable config. See eslint/eslint#3458 They are working on a new config format to address this. See: eslint/eslint#13481

I'm not sure if the fluid-lint-all suffers from similar issues related to "eslint-config-fluid" there is a reference to install it in the fluid-lint-all README which I took as meaning that you should separately install it.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for the explanation, @jobara.

@cindyli
Copy link
Member

cindyli commented Jan 25, 2021

This pull request looks good to me.

Copy link
Member

@BlueSlug BlueSlug left a comment

Choose a reason for hiding this comment

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

This seems to be working fine and all tests are passing locally; I approve!

@jobara jobara merged commit a958887 into fluid-project:main Jan 25, 2021
@jobara
Copy link
Member Author

jobara commented Jan 25, 2021

Merged at a958887

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants