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

Use eslint-config-p5js in a separate ESLint config file for /examples folder #1174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bomanimc
Copy link
Member

In yangsu/eslint-config-p5js#3, I added updated eslint-config-p5js to support the 1.0.0 release of p5.js. Now that the package has been publicly released, we can switch over to it instead of maintaining our own list of p5.js globals.

To make this work, I decided to separate the eslint config files so that I could add the extends: ["p5js"] aspect, which is not supported in ESLint override blocks to my knowledge.

To verify the change, I checked to make sure that the number of lint errors and warnings has not increased as a result of adding the library.

Screen Shot 2021-03-23 at 6 30 46 PM

@bomanimc bomanimc requested a review from shiffman March 23, 2021 22:34
@bomanimc bomanimc changed the title Use eslint-config-p5js in a eparate eslint file for examples folder Use eslint-config-p5js in a separate ESLint config file for /examples folder Mar 23, 2021
@bomanimc bomanimc requested a review from joeyklee April 19, 2021 17:50
Copy link
Member

@shiffman shiffman left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @bomanimc! To be honest, I'm often quite lost when it comes to linting. I see no issues in the pull request itself, seems like the right idea! I checked out the branch and when I run npm run lint, however, I see a lot of errors.

I think a main point of confusion (which we've discussed before) for me is the difference between linting the example and the code library. I think a strict, formal consistent style for the codebase makes sense, but the examples should be allowed to adopt a more informal, p5.js style.

Is this something this pull request is supposed to address or something for us to revisit later?

Screen Shot 2021-04-20 at 10 44 28 AM

@shiffman shiffman mentioned this pull request Apr 20, 2021
@bomanimc
Copy link
Member Author

Hey @shiffman! As it relates to this PR, one of the challenges of using p5.js code is that some of the globally-defined functions like draw, setup, createCanvas, etc are not recognized by the linter, which (without any intervention) will throw lint errors about undefined functions or unused functions. Previously, I address this by manually adding a big list of p5.js functions and global variables to a long file that was imported into our ESLint config. This PR switches that list out for another open-source plugin I contribute to that helps us to clean up this big file in our repository.

The scope of this PR is simply updating our technique for registering p5.js globals, but to your other question about lining, we still have a lot of opportunities and rules to define/ignore for the examples directory. We should open an issue to sort this out! The examples directory will have its ESLint configuration defined in https://github.com/ml5js/ml5-library/pull/1174/files#diff-c66bf1b2f9a7267fbda8c0a1726b830aea61a12b9b5a1de07822e3ac8d2e79c1 after this PR.

@bomanimc
Copy link
Member Author

Created #1190 to address defining linting standards for the examples directory!

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.

None yet

2 participants