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

Add eslint JSDoc checks #819

Merged
merged 5 commits into from Aug 9, 2019
Merged

Add eslint JSDoc checks #819

merged 5 commits into from Aug 9, 2019

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Aug 8, 2019

This PR installs eslint-plugin-jsdoc and enables some of the default rules. I tried to enable only the rules that were consistent with the current use we did of JSDoc.

How to test the changes in this Pull Request:

  1. Run npm run lint and verify there are no errors.

@Aljullu Aljullu added this to the 2.4 milestone Aug 8, 2019
@Aljullu Aljullu requested a review from a team August 8, 2019 11:06
@Aljullu Aljullu self-assigned this Aug 8, 2019
@Aljullu Aljullu added this to In Progress PRs (for automation purposes) in Isotope via automation Aug 8, 2019
@Aljullu Aljullu mentioned this pull request Aug 8, 2019
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

I'm curious, it seems like a lot of the package dependencies (including eslint-jsdoc) are provided by virtue of @wordpress/scripts which in turn exposes @wordpress/eslint-plugin (see here). Would it simplify our package maintenance if we rely on what is exposed by @wordpress/scripts more?

@Aljullu
Copy link
Contributor Author

Aljullu commented Aug 9, 2019

Would it simplify our package maintenance if we rely on what is exposed by @wordpress/scripts more?

Good point, I agree we should try to use upstream configs instead of duplicating them in our repo. Take a look at a6dc1bb and let me know how it looks. 🙂 I had to disable a couple of rules that were generating many errors.

including eslint-jsdoc

If I'm reading it correctly, eslint-plugin-jsdoc was added just a couple of days ago (WordPress/gutenberg#16869) so it's not yet in the last version, right? Anyway, I'm fine using the defaults and once the package updates we will get eslint-plugin-jsdoc enabled in our repo too.

@Aljullu Aljullu requested a review from nerrad August 9, 2019 09:03
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

so it's not yet in the last version, right?

Oops, ya you're right!

Anyway, I'm fine using the defaults and once the package updates we will get eslint-plugin-jsdoc enabled in our repo too.

Works for me :)

I still think our dependency tree has some unnecessary definitions in it (I've commented specifically where). I may be wrong though so push back appropriately 😃

assets/js/hocs/with-searched-products.js Outdated Show resolved Hide resolved
package-lock.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@Aljullu
Copy link
Contributor Author

Aljullu commented Aug 9, 2019

Thanks for the reviews @nerrad. This should be ready for another look.

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

👍 🎉

@Aljullu Aljullu merged commit bc646ab into master Aug 9, 2019
Isotope automation moved this from In Progress PRs (for automation purposes) to 🤖 Done Sprint 22 (July 29 - August 12) Aug 9, 2019
@Aljullu Aljullu deleted the add/eslint-jsdoc branch August 9, 2019 16:18
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.

None yet

3 participants