This only affects javascript files and only affects things
from the prettier whitelist
This will run slowly if you don't have node_modules installed
locally (e.g., in Docker), but it will gladly attempt to
run things in Docker for you.
This adds a new githook_installer image that will install
the githook whenever a docker-compose up happens in the
repo. It will also install the hook whenever a `yarn`
occurs locally (as a postinstall hook).
This commit should also not fail things. For example
having unused variables is an ESLint error, but it isn't
autofixable. It will log the error, but will otherwise
continue. However, it will make this pretty with prettier
as well as fix any other autofixable ESLint errors.
closes CORE-2118
Test Plan:
- Run `yarn`
- Add some semicolons to something from the whitelist
- git add that file
- git commit and it will strip semicolons
- In a dockerized Canvas:
- docker-compose up
- Add semicolons to a file
- git add that file
- git commit, it will take forever (~60s)
- It should have stripped out semicolons
Change-Id: Id9198aa008808e898f29acb9ed64dd14ff843222
Reviewed-on: https://gerrit.instructure.com/171510
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Brent Burgoyne <bburgoyne@instructure.com>
Product-Review: Brent Burgoyne <bburgoyne@instructure.com>
I guess since our team owns these we can opt them in now
Test plan:
* nothing should change
Change-Id: Ic7207e1033869ef60644c41bd5c80a3e8532a6dd
Reviewed-on: https://gerrit.instructure.com/171471
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This commit introduces a better more reasonable ESLint configuration.
It removes all concerns with styling in favor of using prettier to handle
those things.
closes CORE-2096
Test Plan:
- ESLint works
Change-Id: I50b90b7191b576bce4817d885d14f18c4c72d208
Reviewed-on: https://gerrit.instructure.com/170874
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We do this often enough in our actual code that I don’t think it is
worth the noise we create complaining about it. I feel it conditions
people to ignore other stuff that does matter.
I also feel it is a valid thing to do and forcing people to use new
variable names just to appease this rule is not helpful.
Here’s the docs about why the rule exists:
https://eslint.org/docs/rules/no-param-reassign
What say ye’all? Can we axe this?
Change-Id: Ib9e3c54e5a02d3bbc892d875448d71ae87b5e6c1
Reviewed-on: https://gerrit.instructure.com/167894
Tested-by: Jenkins
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this is only needed to protect yourself in hostile environments
where someone might do something like Object.prototype.foobar = “blah”
Because we control all the javascript that is executed on our page,
That won’t ever happen. And if it did, there are a million and a half
other places in our code that will blow up before your code blows up.
Besides, we already catch that ^ in `no-extend-native`
So complaining to you and making you handle this case is just not needed
To read the docs of why this rule exists go here:
https://eslint.org/docs/rules/guard-for-in
What say ye’all? can we axe this?
Change-Id: Ie942578e0b8a3cce8130412f866556da7b9e984c
Reviewed-on: https://gerrit.instructure.com/167896
Tested-by: Jenkins
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
This rule is just there to protect against a pretty esoteric edge
case that we never do. So in effect, in our code, we never will run
into the case where you’ll need it
Read here for why you would need it:
https://eslint.org/docs/rules/no-prototype-builtins
What say ye’all can we axe this?
Change-Id: I4b9124dd457e843e8429eb701e1ddc77fc86c182
Reviewed-on: https://gerrit.instructure.com/167895
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This makes it so everything in app/jsx/account_course_user_search
conforms to our style guide (prettier)
Change-Id: I797121e1002eb1a5bcd28f6a177db3b13ae029eb
Reviewed-on: https://gerrit.instructure.com/159747
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Run ./node_modules/.bin/eslint "app/jsx/**/*.*" | grep 'jsx-a11y'
- Unfortunately you should see some errors :(
Change-Id: I5de6b15471393a7efe5a324b7b55347b7eb34311
Reviewed-on: https://gerrit.instructure.com/155918
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
These don’t objectively catch any bugs. They are just style preferences.
if people really want to do what they complain about, let’s assume they
know what they are doing and not bug them.
Eg: If they think it would be more readable to un-nest their ternary,
great. But if not, like if they are rendering a bunch of jsx lets not
make them make even more verbose and ugly code to not get “no-nested-
Ternaries” errors
And
<Foo ref={e => this.someRef = e} /> should be just fine. It doesn’t
help anything for people to make that more verbose
Change-Id: Ifa3461d8118038a20b42f568f682de3bae9f1f31
Reviewed-on: https://gerrit.instructure.com/153610
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Right now, eslint will tell you your imports should look like:
import bar from ‘compiled/foo/bar’
import {map} from ‘loadsh’
import baz from ‘../baz’
Once we get rid of all the “jsx/*” and “compiled/*” stuff from our
import paths and just use relative paths it will tell you to do:
import {map} from ‘loadsh’
import bar from ‘../../foo/bar’
import baz from ‘../baz’
So it doesn’t make sense to complain about this now and tell people
To put those compiled/foo paths up with the stuff from NPM and not with
the rest of the local stuff.
Note: it _is_ still encouraged to put all your imports in this order:
“nodeJS builtin (eg: import {readFile} from ‘fs’)”
"external (from node_modules)”
“local/relative (eg: ‘compiled/foo’ or ‘../bar’)”
There’s just not an eslint rule that can enforce that for us right now.
Change-Id: I83b0009882d525e9b999b8f9f5991361ce7981fa
Reviewed-on: https://gerrit.instructure.com/151102
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
if you set your editor to eslint --fix on save, which is already safe
be doing, it will also pass anything in this whitelist of dirs to
Opt-in to be prettier formatted too. So you don’t have to wait until
we prettier all the things later and can have consistency among your
now
Test plan:
* gergich should complain if something in app/jsx/permissions is not
Prettier formatted
Change-Id: I438d11f25b10ed58e53656e110aa0d1e7fed5660
Reviewed-on: https://gerrit.instructure.com/149583
Tested-by: Jenkins
Reviewed-by: Venk Natarajan <vnatarajan@instructure.com>
QA-Review: Steven Burnett <sburnett@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
these were all things gergich gives warnings for that everyone ignores
anyway. it is better to turn silly stuff off than to train people to
ignore gergich warnings
Change-Id: I0bc1f807c8c44c0fe6b180126cf8f6e9d14b1797
Reviewed-on: https://gerrit.instructure.com/147815
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test plan:
* Run eslint —fix against a file that doesn’t have a copyright header
(Eg: eslint --fix app/coffeescripts/AssignmentDetailsDialog.js )
* it should add the copyright header for you
* run eslint on a file that already has a proper copyright header
* it should not duplicate it
Change-Id: I0bc05b6cc267fb74909f7a0fe0e93ecda1e2136e
Reviewed-on: https://gerrit.instructure.com/140757
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
This rule was configured to allow for conditional assignment
without forcing destructuring which requires the use of wrapping
parentheses and a lonely semicolon. This style was roundly
rejected after discussion.
Example:
let foo
;({foo} = bar)
Change-Id: I3cfe1be2ac18ce88481367830e2eb30effbc3340
Reviewed-on: https://gerrit.instructure.com/140916
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Jeremy Neander <jneander@instructure.com>
QA-Review: Jeremy Neander <jneander@instructure.com>
This makes prettier errors eslint warnings. So when you
`eslint --fix some/file.js` it will format it with prettier
to try this out, apply this commit and run eslint against your files.
this change will make it so it runs it through prettier and forces
your code to look like prettier would output. But it doesn’t force
Prettier formatting to pass eslint since they are just warnings so if
you really want to format your code differently than prettier you can.
Change-Id: I4f1cdf4962173002a7dc22138be06cc66b842190
Reviewed-on: https://gerrit.instructure.com/131773
Tested-by: Jenkins
Reviewed-by: Jeremy Neander <jneander@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
closes: CNVS-34337
* For now, you can only test things that don’t require any coffeescript
files
* Write your tests using es6 module syntax
* see app/jsx/shared/helpers/__tests__/parseLinkHeader.test.js for an
example
test plan:
* run `yarn run jest`
* you should see some output like:
PASS app/jsx/shared/helpers/__tests__/parseLinkHeader.test.js
PASS app/jsx/actAs/__tests__/ActAsModal.test.js
Test Suites: 2 passed, 2 total
Tests: 5 passed, 5 total
Snapshots: 1 passed, 1 total
Time: 3.072s
Ran all test suites.
* make sure it runs on jenkins build
Change-Id: Ia79cd8ce35dc863c4cc5ce7512998320c7cfdb84
Reviewed-on: https://gerrit.instructure.com/100685
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
…but still force `let foo=1, bar=2` to be:
let foo=1
let bar=2
I don’t actually find any value in enforcing this rule at all and think
we could ignore it altogether but rather than advocating we remove it
completely, this commit just makes it so you can combine uninitialized
variable declarations at the top.
I frequently see this in spec files where we do something like
let subject, result, whatever
//and then in a ‘beforeEach()’ we assign values to those
//and then in each test(..) they are used
What say ye all?
Test plan:
* look at follow-on commit to this that changes MessageStudentsSpec.js
* it should not have eslint errors about:
`error Split 'let' declarations into multiple statements`
Change-Id: I7aafac3a395bac221e9e9b3111a82d0af4671ddb
Reviewed-on: https://gerrit.instructure.com/134822
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
a common pattern to set a ref for a react element is:
<div ref={e => this.refName = e} />
But if you are using our eslint rules (currently set to
"no-return-assign": ["error", "always”]) it will force you to do:
<div ref={e => {this.refName = e}} />
And now, if you are using prettier, it will format that as:
<div ref={e => {
this.refName = e
}} />
That seems needlessly verbose. With this change you can now do:
<div ref={e => (this.refName = e)} />
And both eslint and prettier will be ok with that
Test plan:
* verify functions that return an assignment do what you want in eslint
Change-Id: I56abfdb21a7bf3f5b3eae8088f27749f41f2371c
Reviewed-on: https://gerrit.instructure.com/131788
Tested-by: Jenkins
Reviewed-by: Felix Milea-Ciobanu <fmileaciobanu@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
all of our code that uses AMD looks something like:
define([
‘jquery’,
‘underscore’
], ($, _) => {
$(doStuff)
…
It doesn’t make it any “better” to force people to remove
that blank line at the top of the callback. In fact, leaving it puts
it on par with what we already lint for to *enforce* a blank
line between es6 import or commonjs require() statements and the rest
of your code.
Change-Id: I3dd2051200f6bf458732940306cd044bf631b463
Reviewed-on: https://gerrit.instructure.com/112177
Tested-by: Jenkins
Reviewed-by: brian kirkby <bkirkby@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
…Because we have a ton of existing code that
Does things like `const $user_name = $('#user_name’)`
And it doesn’t make anything “better” to force people to
go around renaming all those. And, although with BEM / React
you probably wouldn’t write code like that in new stuff,
If the DOM ID really is #user_name, for greppability it arguably
is better to keep the js variable and the dom id the same.
(or at least it is not objectively worse than having one with
underscores and one in camelCase).
Change-Id: I042deb3a2bd612eaf74addb141015d288a7330fe
Reviewed-on: https://gerrit.instructure.com/112176
Tested-by: Jenkins
Reviewed-by: brian kirkby <bkirkby@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Closes: CNVS-36236
We now treat code in public/javascripts
exactly the same as app/jsx. Since we pass
it all to babel we can lint with the same rules
as we do app/jsx and you can use all the modern
syntax/features/babel goodness there. There is literally
no excuse to write old crapy javascript anymore.
Now we can start adding some commits that eslint -fix all the existing
stuff in public javscripts.
Test plan:
* if you run eslint public/javascripts/instructure.js it should
give you a bunch of warnings about converting things from var
to let/const and other stuff like that.
Change-Id: I5d400b958ae33f3f13f7f6a9c3505a4f7b5843db
Reviewed-on: https://gerrit.instructure.com/112146
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Breaking those rules will be okay or in the case of
named stuff it fails :)
Change-Id: I5d41ec8d12aee5a59b6f35da0a86ed1571950998
Reviewed-on: https://gerrit.instructure.com/100877
Tested-by: Jenkins
Reviewed-by: Ryan Shaw <ryan@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
closes: CNVS-34124
These are it’s default rules:
"rules": {
"promise/always-return": "error",
"promise/no-return-wrap": "error",
"promise/param-names": "error",
"promise/catch-or-return": "error",
"promise/no-native": "off",
"promise/no-nesting": "warn",
"promise/no-promise-in-callback": "warn",
"promise/no-callback-in-promise": "warn",
"promise/avoid-new": "warn"
}
the only one I changed was set avoid-new to ignore
because I have noticed that there was a few times
where I really did need/want to make my own promise.
I am open to debate on what others think though
Test Plan:
* the eslint errors on your code that uses promises
should have the warnings you expect
Change-Id: I46e5f2d1d2ded2dba1e5aeb2ebcf303bcd4c5981
Reviewed-on: https://gerrit.instructure.com/98201
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Using ENV should not show as a problem anywhere
Change-Id: I4b37a95606755e21716df0dfb61f26a85e295fe9
Reviewed-on: https://gerrit.instructure.com/98198
Reviewed-by: Jeremy Neander <jneander@instructure.com>
Tested-by: Jenkins
Reviewed-by: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
This makes it so eslint handles situations where we don't have
ES2015+ available, while still allowing us to have our own common
rules.
Change-Id: I86b49a32433970afbcc48ef0f6fcdd7301795b63
Reviewed-on: https://gerrit.instructure.com/97872
Tested-by: Jenkins
Reviewed-by: Dan Minkevitch <dan@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Clay Diffrient