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>
Change-Id: Ic510fa4a6b34227cf8c6b01bdeea98d9325d1a0d
Reviewed-on: https://gerrit.instructure.com/136139
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
beginning of line can be dangerous when you copy/paste into irb,
because it's not greedy, so will execute whatever looks like
the first complete statement. this may be an AR query that hasn't
been very restricted yet, accidentally destroying the database
Change-Id: I878e9e364b84ae57d79ea3185d3bddf3e6d56d8a
Reviewed-on: https://gerrit.instructure.com/133054
Tested-by: Jenkins
Reviewed-by: Rob Orton <rob@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Change-Id: I83c54de0e4ec48142c738568d14e54db5766dbbb
Reviewed-on: https://gerrit.instructure.com/131136
Tested-by: Jenkins
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Change-Id: I1aef2dee2089bb61bf220bd8a3640b3c90772188
Reviewed-on: https://gerrit.instructure.com/131135
Tested-by: Jenkins
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
also prevent them in the future. our test-queue fork currently has a bug
where only the last duplicate spec will run, but this is a good idea anyway
most of these are just rewordings, but in some cases delete redundant/
identical specs
test plan: specs
Change-Id: I8f51be55e750cf87ffa8150822f4b80dc99e1eb4
Reviewed-on: https://gerrit.instructure.com/103993
Tested-by: Jenkins
Reviewed-by: Landon Wilkins <lwilkins@instructure.com>
Product-Review: Landon Wilkins <lwilkins@instructure.com>
QA-Review: Landon Wilkins <lwilkins@instructure.com>
Change-Id: I380b57bc6ee4e7b8e606e5148fb0468031ccee57
Reviewed-on: https://gerrit.instructure.com/101538
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins
Product-Review: Jon Jensen <jon@instructure.com>
QA-Review: Jon Jensen <jon@instructure.com>
* before(:each) is nice since it's often right next to before(:once)
* RMessageSpies isn't a thing
* don't complain about big blocks (e.g `describe`)
Change-Id: I296433cf2478ac3a304a2e20a1a3cd5de6515678
Reviewed-on: https://gerrit.instructure.com/101502
Tested-by: Jenkins
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: Jon Jensen <jon@instructure.com>
QA-Review: Jon Jensen <jon@instructure.com>
closes CNVS-34657
test plan: rubocop should still work
Change-Id: Ibdd340dd7ba516639ee5b7225449422b324bd33a
Reviewed-on: https://gerrit.instructure.com/100831
Tested-by: Jenkins
Reviewed-by: Tyler Pickett <tpickett@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
This fixes the rule setting the limit for nested example groups in
RSpec. Its current setting is invalid and does not work.
test plan:
* verify Jenkins passes
Change-Id: If6e03df1d9fd66bc5aacf1f78e0918086c6c73de
Reviewed-on: https://gerrit.instructure.com/100780
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins
QA-Review: Jeremy Neander <jneander@instructure.com>
Product-Review: Jeremy Neander <jneander@instructure.com>
Change-Id: Ib916de654fa303c2c26d17ca23c6e113d212fa5f
Reviewed-on: https://gerrit.instructure.com/100204
Tested-by: Jenkins
Reviewed-by: Rob Orton <rob@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
This disables the HttpPositionalArguments rubocop rule, which is invalid
for versions of Ruby prior to Ruby 5. This also sets the limit for
nested example groups in RSpec to 3, which is a more reasonable limit
than the default of 2.
test plan:
* verify Jenkins passes
Change-Id: I8f2badb369d6932923aa4c096c9b9014be19805a
Reviewed-on: https://gerrit.instructure.com/100198
Tested-by: Jenkins
Reviewed-by: Rob Orton <rob@instructure.com>
QA-Review: Jeremy Neander <jneander@instructure.com>
Product-Review: Jeremy Neander <jneander@instructure.com>
Change-Id: Ice28cad151efa8f58e45b88eb3b11ac248d07c7b
Reviewed-on: https://gerrit.instructure.com/100104
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Change-Id: I185cc5dbfaafe64f74a511aac1ab9b94bae38d63
Reviewed-on: https://gerrit.instructure.com/98776
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
This cop encourages explit require_dependency calls for ambiguous nested
constants in specs. What does that mean? Consider:
module Analytics
describe Assignments do
Depending on what has been required and/or autoloaded, `Assignments` could
either resolve to `Analytics::Assignments`, or just the top-level
`Assignments`. This is a cause of flickering failures and test-queue woes.
This example should either have an explicit `require_dependency` call, or
get rid of the module and just do `describe Analytics::Assignments``
Correct all existing ones in the codebase, except for a few
ActiveRecord-related ones (the auto-correct isn't quite perfect, i.e. it
assumes the file path will be `const_name.underscore`, which is no bueno
for things like ActiveRecord::ConnectionAdapters::PostgreSQLAdapter)
Test plan:
n/a, specs
Change-Id: Ic24bf3e0f547ca11c46887d4af92804da091912a
Reviewed-on: https://gerrit.instructure.com/98752
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
Product-Review: Jon Jensen <jon@instructure.com>
QA-Review: Jon Jensen <jon@instructure.com>
note: Astrolabe::Node is now just Rubocop::Node
closes CNVS-34077
test plan: rubocop should still lint correctly
Change-Id: Id5feb1700ad78cc0b34c6cb90d2a75320b210138
Reviewed-on: https://gerrit.instructure.com/98512
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
Product-Review: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
This reverts commit 2073553507.
This reverts commit 56381d06b4.
rubocop upgraded today, and we tried a couple times to fix it, but since
we only run rubocop when ruby files change, the fixes themselves passed
the build but didn't fully fix the problem. for now, until we can
complete an upgrade, just pin the known working version of rubocop.
Change-Id: Ib7587e5ff9f3c00817f920506208a0e20444d379
Reviewed-on: https://gerrit.instructure.com/70356
Reviewed-by: Jon Jensen <jon@instructure.com>
Tested-by: Jenkins
Product-Review: Jon Jensen <jon@instructure.com>
QA-Review: Jon Jensen <jon@instructure.com>
rubocop wasn't version pinned, so jenkins got a later version,
and the aux build started failing because of a config difference.
this fixes the config and pins the version of rubocop
Change-Id: I5d94b779a8d29dac66ad0e121737816459c62ba3
Reviewed-on: https://gerrit.instructure.com/70337
Reviewed-by: Jon Jensen <jon@instructure.com>
Product-Review: Ethan Vizitei <evizitei@instructure.com>
QA-Review: Ethan Vizitei <evizitei@instructure.com>
Tested-by: Ethan Vizitei <evizitei@instructure.com>
Style/ParallelAssignment has the wrong namespace - should be Performance
Change-Id: Ic93ee267b5c2d2f2240eefdd7eb4180f3298459e
Reviewed-on: https://gerrit.instructure.com/68668
Reviewed-by: Jon Jensen <jon@instructure.com>
Product-Review: Jon Jensen <jon@instructure.com>
QA-Review: Jon Jensen <jon@instructure.com>
Tested-by: Jenkins
The modifier while/until construction should not be preferred.
The style guide has been updated to reflect this:
https://gollum.instructure.com/Ruby-style-guidelines#While/Until
Change-Id: Ib1b8b3cf29d52729ba9d3f9066554fc421d47111
Reviewed-on: https://gerrit.instructure.com/52997
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
Product-Review: Strand McCutchen <smccutchen@instructure.com>
QA-Review: Strand McCutchen <smccutchen@instructure.com>
Cody Cutrer