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 to enforce code quality + styling #5635

Closed
wants to merge 40 commits into from

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Jan 31, 2020

This PR introduces ESLint to help enforce code quality. Based upon its use in Stumptown, we had verbally discussed (at All Hands 2020) implementing ESLint into BCD. ESLint can integrate with Prettier as well, so we don't need to run Prettier separately -- it's all integrated right into ESLint's command. Fixes #5252.

The ESLint configuration is set to extend the default recommendations from ESLint, with some slight alterations to adhere to our code styling, such as single quotes, allowed use of the console, always end lines with semicolons, etc. As an additional feature, ESLint is also configured to enforce JSDoc comment linting.

Note: this PR will fail for now as there are many errors in our existing code to fix. I plan to follow up with modifications that fix these issues later on, after all other linter and infra PRs are merged or closed. This is marked as "not ready" until there are no issues.

@queengooborg queengooborg requested a review from Elchi3 as a code owner January 31, 2020 13:47
@ghost ghost added the dependencies Pull requests that update a dependency package or file. label Jan 31, 2020
@ghost ghost added infra Infrastructure issues (npm, GitHub Actions, releases) of this project linter Issues or pull requests regarding the tests / linter of the JSON files. labels Jan 31, 2020
@queengooborg queengooborg added the not ready This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Jan 31, 2020
@ghost ghost added the scripts Issues or pull requests regarding the scripts in scripts/. label Mar 16, 2020
@ddbeck
Copy link
Collaborator

ddbeck commented Mar 25, 2020

I'm sorry, @vinyldarkscratch, but I am having a hard time seeing this getting merged. I'm going to close this, but I have some ideas for next steps.

The real problem is that #5252 is under specified. I think it's likely there's a consensus for using ESLint in general, but the configuration and plugins and dependencies needs a complete proposal to grapple with, or needs to be broken down into smaller pieces that we can tackle one-by-one (e.g., whether to prefer arrow functions). Looking at this PR, I see a lot of changes, but I don't really understand (or know how I would understand) how every piece is necessary to satisfy #5252.

To get this closer to resolution, I think we need to go back to #5252 and spell out the expectations more clearly. What code quality problems do we have in BCD? Does ESLint find those problems (either by default, or with additional config)? And what would fixing those problems entail? Then we can have a process we can work through, and we'll be able to have PRs with a clear basis for reviewing them.

I know there are some other PRs that relate to this and closing this likely impacts them. I'll follow up on those PRs known to me shortly.

Again, I'm sorry to be closing this. I know you've put some real effort into this and I'm appreciative of what you've done. I imagine what you've learned here will help us get to the point of eventually being able to merge something like this, but there's going to be a bit of a learning process first. Thank you.

@queengooborg
Copy link
Contributor Author

I totally understand, no worries! So to summarize, we should proceed with some more discussion first, before looking at getting a PR going?

(I'll also be keeping this branch around in my own fork, so that it can be quickly adjusted at any time based upon what we discuss!)

@Elchi3 Elchi3 removed their request for review April 8, 2020 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency package or file. infra Infrastructure issues (npm, GitHub Actions, releases) of this project linter Issues or pull requests regarding the tests / linter of the JSON files. not ready This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. scripts Issues or pull requests regarding the scripts in scripts/.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ESLint to enforce code quality
2 participants