-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fix: enforce style rules #99
base: master
Are you sure you want to change the base?
Conversation
493a29f
to
7feeacd
Compare
7feeacd
to
62c994d
Compare
@@ -39,6 +39,9 @@ jobs: | |||
- name: Run jshint | |||
run: npm run jshint | |||
|
|||
- name: Run fmt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably shouldn't format here but make sure that formatting already is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause it to fail if it's not correct. This enforces that the style is followed.
If you don't run this, you can't enforce that it was done before (unless you do something else that's effectively the same).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it doesn't format itself but only checking that the format is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. It will exit 1
if there are any changes and exit 0
if there are no changes.
@@ -1,42 +1,37 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use eslint + airbnb style for all JS packages. Shall we just move to eslint + prettier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier is definitely the standard for JS.
ESLint is more for TypeScript and ECMAScript and other JS derivatives. JSHint is for JS.
ESLint can operate on JS, but it has a boatload of dependencies, very complex configuration, etc.
I personally won't run eslint
because it's too risky and insecure due to the supply chain vulns', but if CI/CD can run and report it... meh. Still, there should be a way to apply ESLint that is not incompatible with just JavaScript, and therefore not incompatible with JSHint.
That said, ESLint is better than nothing, but it's nowhere near as good as JSHint for JavaScript. JSHint is a single dependency, no plugins, no formatting, no rewrites, no style, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most important here is a consistency. All JS repos in this org should follow the same code style. Then we should go with the same configuration for eslint what we in https://github.com/dashpay/platform/ and prettier should follow eslint config https://github.com/prettier/eslint-config-prettier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All JS repos in this org should follow the same code style
- they don't presently
- this fixes this repo as this repo is presently set up
- it's a much bigger lift to do the work to transition it to eslint
- Opening a can of worms...
Why move from something that has just 2 dependencies to something that has hundreds of dependencies? There are just too many things that go wrong. Too many versions to track. Slower CI/CD times.
If someone else wants to bring in the complexity and then make all the corresponding updates, then let them do that in another PR to transition the repository. But for now, let's just fix what's here.
I don't use eslint because I value both security and ease of use. eslint
is an absolute monster. I won't install it on my computer because I'm morally opposed to it - it encourages non-standard features, supply chain security attacks (which have been exploited), and overall bloat.
Several style rules were defined for
.jshintrc
, however, they have long since been deprecated and removed fromjshint
, and were not being enforced.I transitioned these to
.editorconfig
and.prettierrc.json
and added checks to enforce them during the test phase.