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

Adopt and enforce a common ES6 coding standard #138

Open
rod-glover opened this issue Feb 19, 2018 · 6 comments
Open

Adopt and enforce a common ES6 coding standard #138

rod-glover opened this issue Feb 19, 2018 · 6 comments

Comments

@rod-glover
Copy link
Contributor

Code style in this repo is both internally inconsistent and inconsistent with the most widespread JS style standards. There are several places where the code does not even follow elementary problem-preventing conventions, such as using === and !==.

Suggest we adopt a style standard, a linter to check it, and a policy or tools to enforce it. I recommend the following:

ESLint:

(ESLint plus the react plugin is installed on my IDE and has proved extremely useful for helping me spot possible errors and stick to React best practices. It's also calling out our code A LOT.)

I'd be happy to lead the effort on this, including applying the auto-fix tool.

@corviday
Copy link
Contributor

Seems like a good idea. I previously tried to use JSHint with the CE frontend code, but it didn't understand React or JSX and threw up so many bogus JSX-related errors I gave up on it.

@jameshiebert
Copy link
Contributor

I support this as well. We actually do have a command in the repo to configure and run eslint.

But as you have pointed out, the problem is that it's not run automatically and regularly with each commit, so the standards aren't actually enforced.

@rod-glover
Copy link
Contributor Author

rod-glover commented Feb 20, 2018

We also have two git hooks defined, one to run eslint before commit (using the command you noted), and the other to rebuild node_modules whenever a new commit is checked out.

They are inactive because they are not in the .git/hooks directory (instead in git/hooks). It would be easy to move the eslint hook into .git/hooks.

I'll do that locally to check whether it works (looks like it should). If that seems good, I'll post a PR to enable the eslint githook.

@jameshiebert
Copy link
Contributor

They are managed in git/hooks because one cannot place things in the .git directory to be managed by the .git directory :) So it's up to whoever checks out the repo to do the manual step of copying them over (or symlinking them into .git).

@jameshiebert
Copy link
Contributor

We could do something like this...

@rod-glover
Copy link
Contributor Author

Small complication: The .git subdirectory is ignored and is not part of the repo. (This is standard, and widely noted in discussions of git hooks.) Therefore the easy path is also voluntary, i.e., not externally enforced: Copy the hooks you want activated into your local .git/hooks directory. I will set this up (a slight bit of additional scripting in git/hooks.)

Strong enforcement on the server side is kind of a pain. But there is a simpler form of enforcement we can employ: Add a line to .travis.yml to run eslint as well as the unit tests. If eslint fails, the branch is not considered passing. There would be a little additional scripting to do this, but the outline of what's needed is in existing examples.

corviday added a commit that referenced this issue Nov 23, 2018
@rod-glover rod-glover added this to the Triage 2020-07 - Priority 1 milestone Jul 8, 2020
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

No branches or pull requests

3 participants