because this isn't even pointless pedantry. it's just pointless
Change-Id: I7b8cd656a4adb8da3b5e30a59593297f91d9aa9a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/286846
Reviewed-by: Aaron Ogata <aogata@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
seems overly pedantic
Change-Id: Ibe3ccd76978650842dd09fd1232df5673e292d58
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/287118
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
to allow the modifier form, e.g. this is okay
if condition_a || condition_b
do_something unless condition_c && condition_d
end
and is in fact preferable to
if (condition_a || condition_b) && !(condition_c && condition_d)
do_something
end
which is what rubocop would turn it into otherwise
Change-Id: I1b917e3ea5149379f22e70294d263b713b3b9b35
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/282475
Reviewed-by: Eric Saupe <eric.saupe@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
because it gives bad advice. `str.casecmp("foo").zero?` is not only
less clear than `str.downcase == "foo"`, but it is not Unicode-aware
(ironically, `str.casecmp?("foo")`, with the question mark, _is_
Unicode-aware)
Change-Id: I1abd17f4a09b205073998c5cf9e89cb4bd85ad6b
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/282294
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
test plan:
- create a migration (locally) and add it to a commit
(no need to actually commit, push, or even run the migration)
that adds a foreign key to an existing table, e.g.
add_foreign_key :users, :eportfolios
- run script/rlint, and it should complain that you should
use a non-transactional migration and add `delay_validation: true`
to the arguments
- do those things, run script/rlint again, and it should not
complain
- add a foreign key to a table created in the same migration,
e.g.
create_table :foo do |t|
end
add_foreign_key :foo, :bar
- run script/rlint again, and it shouldn't complain
flag=none
closes DE-940
Change-Id: I3f235cbf1ff4011e20d74bdaa8953b857c7e330a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/280514
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Isaac Moore <isaac.moore@instructure.com>
QA-Review: Isaac Moore <isaac.moore@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
since planned cops may provide different severity levels
depending on circumstances
refs DE-942
flag = none
Change-Id: I7501c13ea350241c47debceb7440cc8ce3d43925
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/280532
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Isaac Moore <isaac.moore@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
it's removing disable directives that need to be there
Change-Id: Iecff7d5559742af834946a6d9223fd24634baa90
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/279693
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
[skip-stages=Flakey]
manual. obvious duplicates were removed. others I looked for context on what
the original copy/paste error was.
Change-Id: I6d009c480eacf9233ed0c8253ce715bc2f74e8bf
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/279432
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
[skip-stages=Flakey]
manual, mostly by fixing copy/paste name problems,
combining groups that didn't need separating, or creating
new groups to contain contiguous top level groups that can't
be combined due to having different setup
Change-Id: I5b21664eabd8c3a62e22c09a28ba8bf519371aa5
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/279428
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
means we can remove a whole bunch of specific cop configurations that
now inherit the proper settings. cops that still have violations need
to be explicitly named to make them not error. this means that the
list of remaining cops is explicitly in our config now. also combined
the don't-auto-correct lists with the not-yet-fixed lists, so that
it's only a "permanent" section and a "todo" section
the final computed configuration is unchanged by this commit, with one
exception - Bundler/OrderedGems was moved to the Gemfile.d/.rubocop.yml
override, and some of the internal gems violated that so their
gemfiles have been autocorrected
Change-Id: I100884a96dbe9eb9d4bfc0692663c271daa9de16
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/279413
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
we're far enough along that the generated disablings are no longer useful
Change-Id: Iec341384f7a5f6eeb42bad4ae1ea12f433d270a4
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/279397
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
it was just too confusing on which one an editor is using, double comments
in jenkins, etc.
this is accomplished by several things:
* required cops are just marked as severe, instead of using a separate
config for them, and failing if anything shows up from that config
* get rid of all the logic to only include certain directories for
certain cops. turns out it's not _that_ ominous to correct errors
across the entire repository before marking a cop as required.
* but still auto-generate config to turn _off_ autocorrect for
non-severe cops. this is important because auto-correct must run
for entire files, and we don't want it auto-correcting optional
things that you didn't touch.
* update gergich to get more details from the parsed comments.
this plus the prior point means we _don't_ have to have heavy mode when
in autocorrecting, but we still display out-of-context lines that were
autocorrected
this also makes it so we can use per-dir .rubocop.yml files again, so
take some of the exceptions out of the root and put them in their own
directory
Change-Id: Ie936d1a9920b68910acd250ba817c7b4a670b958
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/274394
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
document why each cop is disabled, disable a few more globally, and document
the intentions for each currently disabled cops in the optional config
Change-Id: I7abaabd76142a61e1922f55da0c8abb6a665ec05
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/274265
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
* remove spurious .rubocop.yml override files
* split the configuration into an enforced and optional
* run both configurations in jenkins (may result in some duplicate
comments at different levels)
* auto-correct the enforced configuration in the pre-commit hook
* fix comments for Gemfile.d and the root dir; enforced configuration
is only applied to that directory for now
Change-Id: I8da21073d74e19138b1b580d66c7aae6465348d4
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/273898
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
it's disabled anyway, and because the cop requires active_record itself,
it adds 0.5s to the boot time of rubocop
Change-Id: Ie22944afef9df45fe08ea0e13857a4703d1c53f8
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271959
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
Reviewed-by: August Thornton <august@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
and rubocop-rails was split out to a dedicated gem
Change-Id: I12b45540a400aab0d57db303457103516ab92df7
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271931
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
they make our rubocop SnR waaaay too bad right now that we ignore everything
Change-Id: Ie0145be67ee5aec54692f4c463f2bd2c2ab41518
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/260505
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
refs FOO-1644
when other engines need migrations, we have to
be able to move "tagged" migrations down to them.
Letting outrigger manage the dependency
is nice since we extracted it from canvas anyway.
TEST PLAN:
1) nothing about migrations changes
Change-Id: I93d2683cc05584ed9bf871b61e06ba39b70a3f5d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/260370
Reviewed-by: Rob Orton <rob@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Ethan Vizitei <evizitei@instructure.com>
Product-Review: Ethan Vizitei <evizitei@instructure.com>
Change-Id: I5c0b7625bb22e4e285040201cddb8ab330f494bc
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/251306
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
this changes upgrades to the earliest version of rubocop
supporting ruby 2.6 while also choosing a version that fixes
a breaking 0.60.0 change to revert psych version change.
namespace changes were made on multiple cops to support
the new rubocop version requirements as well as the change to
move performance cops out of the rubocop gem in to
rubocop-performance.
the Layout/IndentFirstParameter cop is currently broken but a
rubocop version upgrade will fix that. the FreezeConstants cop
also required a fix to handle nil types.
refs DE-289
test plan:
- ensure rubocop runs completely
Change-Id: I0ca68f426e287b5a973b96a4ae0e975f1cbc0240
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/248786
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Kyle Rosenbaum <krosenbaum@instructure.com>
Product-Review: Kyle Rosenbaum <krosenbaum@instructure.com>
Reviewed-by: Ryan Norton <rnorton@instructure.com>
closes#1565
test plan: rubocop still works
Change-Id: I77752b64224e176e5a4aff6d470cd85354b05474
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/225005
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Rob Orton <rob@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
some hot takes:
- 4.times.map is more readible to Array.new(4).map and has no perf gain
in small cases which is the overwhelming use case for specs. This
also has no perf impact for app code since it's excluded for specs only.
- change { AnonymousOrModerationEvent.count } is more readible than
change(AnonymousOrModerationEvent, :count) because the former is the
exact code you would see in your app code. This is an RSpec only cop
so it has no impact on app code.
Change-Id: I6f70b6a9f69a3375cf6f8db27eadd810f25361ca
Reviewed-on: https://gerrit.instructure.com/162540
Tested-by: Jenkins
Reviewed-by: Spencer Olson <solson@instructure.com>
Reviewed-by: Cameron Matheson <cameron@instructure.com>
Product-Review: Derek Bender <djbender@instructure.com>
QA-Review: Derek Bender <djbender@instructure.com>
We're already ignoring it in RSpec and Selenium tests. And Pact
tests inherently have a lot of lines.
Change-Id: Ia976224a03426864beb04d885011e982d4c2a60d
Reviewed-on: https://gerrit.instructure.com/156583
Reviewed-by: Michael Hargiss <mhargiss@instructure.com>
Tested-by: Jenkins
Product-Review: Tucker McKnight <tmcknight@instructure.com>
QA-Review: Tucker McKnight <tmcknight@instructure.com>
to be the one we used most often
Change-Id: I0ed597588f48d2d3f57111c5a409b1304ebab666
Reviewed-on: https://gerrit.instructure.com/143446
Reviewed-by: Rob Orton <rob@instructure.com>
Product-Review: Rob Orton <rob@instructure.com>
QA-Review: Rob Orton <rob@instructure.com>
Tested-by: Jenkins
yes, some of them are kind of sad/bad. but the signal to noise with them
on is so bad that we end up ignoring far more useful advice because
we ignore gergich completely
Change-Id: Ie62b9e4c6459440c7c6cfaa29bae8271be639d77
Reviewed-on: https://gerrit.instructure.com/141069
Reviewed-by: Rob Orton <rob@instructure.com>
Tested-by: Jenkins
Reviewed-by: Simon Williams <simon@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
also fix a typo in .eslintignore caused by cae5d84950
motivation:
https://github.com/bbatsov/rubocop/issues/4222#issuecomment-290655562
this is a deliberate idiom in rspec, so let's update .robocop.yml to
match rubocop's own config
closes: GRADE-841
test plan: specs pass
Change-Id: I534eabe48fb227b6fa7b425502e645f22b1a51f3
Reviewed-on: https://gerrit.instructure.com/140424
Tested-by: Jenkins
Reviewed-by: Keith T. Garner <kgarner@instructure.com>
Product-Review: Derek Bender <djbender@instructure.com>
QA-Review: Derek Bender <djbender@instructure.com>
rubocop changed the namespace on some items. This updates to the new
names so that it won't toss out warnings that things have moved.
Change-Id: Ie9003672a10adebab983b1b91393cc83a207b3c3
Reviewed-on: https://gerrit.instructure.com/136688
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins
Product-Review: Keith T. Garner <kgarner@instructure.com>
QA-Review: Keith T. Garner <kgarner@instructure.com>
Jeremy Stanley