Update RuboCop support; use Bixby style guide #2655

no-reply · 2018-02-16T15:03:18Z

Bixby is a RuboCop style guide that decouples (to the degree possible) RuboCop
software updates from unrelated style guide updates. Bixby disables all rules by
default, and re-enables rules that are adopted in the community in general.

Bixby's promise is to avoid adding rules without bumping major
version. Pinning to the current v1.0.0 will allow us to update software versions
with reduced changes (ideally none) to code style. Enforcement of existing rules
may still change to become more strict based on updates to rule enforcement
code.

Several rule exceptions and other configurations that I could identify as being
inherited from Bixby are removed from the local .rubocop.yml.

Closes #2568.

Changes proposed in this pull request:

Update RuboCop dependency to Bixby
Fix style violations to align with current style guide

@samvera/hyrax-code-reviewers

jcoyne · 2018-02-16T15:30:26Z

app/actors/hyrax/actors/model_actor.rb

-      def update(env)
-        model_actor(env).update(env)
-      end
+      delegate :update, to: :model_actor


Maybe I'm tired, but how does this work? How does model actor get it's parameter passed to it?

huh. I think this was a RuboCop autocorrect. I wonder if this might be a bug in the autocorrect code. If it passes CI, I'll look into it. If it fails, I'll revert the change and fence the rule.

Thanks for the thorough review.

Sure enough: rubocop/rubocop#5393

jcoyne · 2018-02-16T15:32:58Z

app/helpers/hyrax/hyrax_helper_behavior.rb

@@ -1,3 +1,4 @@
+# coding: utf-8


Why? Isn't this the default in ruby 2.x?

This might be an editor artifact from me. I'll remove it.

jcoyne · 2018-02-16T15:34:51Z

app/services/hyrax/collection_types/create_service.rb

@@ -133,7 +133,7 @@ def self.add_participants(collection_type_id, participants)
            agent_id = p.fetch(:agent_id)
            access = p.fetch(:access)
            Hyrax::CollectionTypeParticipant.create!(hyrax_collection_type_id: collection_type_id, agent_type: agent_type, agent_id: agent_id, access: access)
-          rescue => e # rubocop:disable Lint/RescueWithoutErrorClass
+          rescue => e


I really wish we called this out as something that should be fixed. Can we add this cop to Bixby?

This cop is new since bixby launched. I've opened a bixby ticket, but it will be a 2.0 feature.

In the meanwhile, we can add this cop locally. However, I'm not sure what the value of doing that is at the moment, if we are just going to disable it in the individual cases.

@jcoyne This cop has also been replaced with Style/RescueStandardError.

Honestly, I'm looking at this now and pretty unconvinced that this is a Good Cop ™️. In this case, we're catching StandardError and logging, which seems like an extremely common and happy use case.

I disagree that this is a happy use case. Currently I have no idea what error we're expecting here. Could it be a KeyError or a Database error (out of space maybe?) I'd really like for those exceptions to be bubbling up.

I've added #2666 to address this case in particular, but I'd really think that over general error catching is likely a code smell.

I opened a Bixby issue at samvera/bixby#23; but resolving it will be a major bixby version. I'd suggest we discuss there for a bit.

If we want to add this locally in the meanwhile, I can do that. I'd prefer not to block this PR for it, since the only current instance of the issue is now fixed.

catching StandardError is IMO fine to do when you are not actually swallowing the error but: a) logging and re-raising original, and/or b) re-raising another module-specific error wrapping the original.

I do both frequently. It would be annoying if I had to tell rubocop to let me do it all the time, but then, that may just be the way it is with rubocop.

At any rate, whether to add or remove a rule to Bixby definitely seems like a different issue, yes?

jcoyne · 2018-02-16T15:35:03Z

hyrax.gemspec

@@ -1,3 +1,4 @@
+# coding: utf-8


jcoyne · 2018-02-16T15:35:50Z

spec/helpers/hyrax/ability_helper_spec.rb

@@ -6,7 +6,7 @@
      Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC =>
        "<span class=\"label label-success\">Public</span>",
      Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED =>
-        "<span class=\"label label-info\">%s</span>",
+        "<span class=\"label label-info\">%<name>s</span>",


What is going on here?

See: http://www.rubydoc.info/github/bbatsov/RuboCop/RuboCop/Cop/Style/FormatStringToken

Okay, looks good.

jcoyne · 2018-02-16T15:36:28Z

spec/services/hyrax/qa_select_service_spec.rb

@@ -1,7 +1,6 @@
 RSpec.describe Hyrax::QaSelectService do
  let(:authority) do
    # Implementing an ActiveRecord interface as required for this spec
-    # rubocop:disable RSpec/InstanceVariable


This is a cop that I wish we could maintain. Can we add to Bixby?

It is enabled here: https://github.com/samvera-labs/bixby/blob/7d62f7ce8e7ea6d22e4478dce5c8af4baddeff98/bixby_rspec_enabled.yml#L66

I think RuboCop must have made this cop less sensitive to class definitions within RSpec. This has been a pain-point for me for cases like this for some time, so I'm grateful to find that these cases are no longer flagged.

See: rubocop/rubocop-rspec#506

jcoyne · 2018-02-16T15:39:46Z

My impression is that packing conditions and expressions (especially when both are complex) onto the same line actually reduces the readability of the code. I'm interested to know what others think.

no-reply · 2018-02-16T15:51:15Z

@jcoyne to clarify, you are arguing to drop enforcement of this rule? https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier

jcoyne · 2018-02-16T16:10:35Z

@no-reply that looks like the right cop.
While I generally agree with their simple examples:

# bad
if some_condition
  do_something
end

# good
do_something if some_condition

In our real code, I don't think that this:

@parent_presenter ||= show_presenter.new(search_result_document(id: params[:parent_id]), current_ability, request) if params[:parent_id]

is an improvement over this.

if params[:parent_id]
   @parent_presenter ||= show_presenter.new(search_result_document(id: params[:parent_id]), current_ability, request)
end

no-reply · 2018-02-16T16:14:12Z

@jcoyne That makes sense. I tend to think this kind of sneaky nested conditional is pathological to begin with, but I'm not sure that should stop us from disabling the rule.

I'm a fan of the rule in general, but open to consideration. I'd accept discussion on a Bixby ticket and/or PR. In the meanwhile, I'm open to blocking this ticket on a local disable of the cop.

jcoyne · 2018-02-16T16:50:01Z

@no-reply I totally agree that those lines could have their readability improved in other ways, but I think we shouldn't make the current situation less readable while we wait for a refactor that may or may not ever come.

no-reply · 2018-02-16T16:50:47Z

I think issues are now fixed with the exception of open discussions:

no-reply · 2018-02-16T16:52:04Z

@jcoyne If that refactor came on a follow up commit on this PR, could we avoid disabling the rule?

Or would other concerns about that rule's value remain?

no-reply · 2018-02-16T17:04:14Z

I really have no idea why the Ruby 2.1 syntax linter suddenly has unexpected tokens on perfectly reasonable Ruby code. I was able to reproduce locally with bundle update...

laritakr · 2018-02-16T20:46:06Z

@no-reply Ruby 2.1 linter is failing based on https://github.com/whitequark/parser version 2.5.0.0 (a Rubocop dependency) Mine was running fine under 2.4.0.2. It failed when I updated it locally. We're pinning it to get travis to build again.

Bixby is a RuboCop style guide that decouples (to the degree possible) RuboCop software updates from unrelated style guide updates. Bixby disables all rules by default, and re-enables rules that are adopted in the community in general. Bixby's promise is to avoid adding rules without bumping major version. Pinning to the current v1.0.0 will allow us to update software versions with reduced changes (ideally none) to code style. Enforcement of existing rules may still change to become more strict based on updates to rule enforcement code. Several rule exceptions and other configurations that I could identify as being inherited from Bixby are removed from the local `.rubocop.yml`. Closes #2568.

I can't see how nesting conditional assignment of the same variable does anything here; pushing this through CI to see if I'm wrong.

The version of RDF.rb checked in this conditional is no longer an allowed dependency. ActiveTriples 11.0.0 and up require RDF.rb >= 2.0.0.

no-reply · 2018-02-19T00:37:34Z

@jcoyne I went ahead and refactored to remove the @param ||= do_something if should_do? occurrence. There are still some conditional assignments like @param = do_something if should_do?, but those generally feel pretty readable to me. Thoughts?

I also removed an unnecessary check for an RDF.rb version that isn't allowed by the dependency tree.

jenlindner · 2018-02-22T13:42:11Z

@jcoyne were your concerns addressed? it would be really helpful to get this PR in, can we accept most of it and make issues of any remaining concerns?

jcoyne · 2018-02-22T13:47:46Z

@jenlindner No, not really. I still don't agree that we should allow anyone to raise StandardError (without some justification as to why there isn't a more specific error that is more suitable). I also think that formatting the conditionals to the end often hurts readability. But I'll not want to block this PR if others think this is preferable.

jenlindner · 2018-02-22T13:58:16Z

@jcoyne Yeah, it's not like there's no merit to that position, there clearly is. I don't want to see the discussion get lost and rubocop left in a state that also opens us up to trouble/tech debt, which it might be now? I'm not really sure about that, but we do need a working rubocop and most of the other improvements seem uncontested. I think how to handle errors and where to catch/log/raise is a really great topic for code quality/stability discussions and would love to have the dangers of over-catching presented in them.

Thank you.

@jrochkind how are you feeling about the PR as it is? would you be open to contributing to a discussion about error handling in code quality/stability?

no-reply · 2018-02-22T21:31:01Z

@jcoyne:

I still don't agree that we should allow anyone to raise StandardError (without some justification as to why there isn't a more specific error that is more suitable).

How does this relate to the discussion at samvera/bixby#23 (comment)?

I also think that formatting the conditionals to the end often hurts readability.

I thought that we had refactored the cases that were problematic wrt this issue?

It's not clear to me what work would unblock this PR at this point. Should I simply close it?

cjcolvar · 2018-02-23T00:31:35Z

@no-reply I think it is valuable to keep it open. How about we talk about it on the tech call if it isn't resolved by then? Or take it to the email list? Either way it would be good to get more voices on this.

no-reply · 2018-02-26T17:55:21Z

@samvera/hyrax-code-reviewers @jcoyne has indicated he doesn't intend to be blocking this PR. What can we do to move forward?

jenlindner · 2018-02-26T18:05:57Z

@cjcolvar @no-reply I added this item to the tech call agenda: https://wiki.duraspace.org/display/samvera/Samvera+Tech+Call+2018-02-28.

cjcolvar

👍 Final rubocop rule changes can be done on bixby and there weren't any blockers brought up on the tech call.

Update RuboCop support; use Bixby style guide

no-reply added the in progress label Feb 16, 2018

jcoyne reviewed Feb 16, 2018

View reviewed changes

no-reply mentioned this pull request Feb 16, 2018

Add Lint/RescueWithoutErrorClass samvera/bixby#23

Closed

no-reply force-pushed the rubocop branch 2 times, most recently from c783f2f to 9f1c147 Compare February 16, 2018 16:47

no-reply force-pushed the rubocop branch from 9f1c147 to 6ddae60 Compare February 18, 2018 16:06

Tom Johnson added 2 commits February 18, 2018 16:25

Remove nested conditional assignment

698c88d

I can't see how nesting conditional assignment of the same variable does anything here; pushing this through CI to see if I'm wrong.

Remove check for RDF.rb version < 2.0.0

e2d55fa

The version of RDF.rb checked in this conditional is no longer an allowed dependency. ActiveTriples 11.0.0 and up require RDF.rb >= 2.0.0.

cjcolvar approved these changes Feb 28, 2018

View reviewed changes

no-reply merged commit 8406c1f into master Feb 28, 2018

no-reply deleted the rubocop branch February 28, 2018 18:00

no-reply removed the in progress label Feb 28, 2018

elrayle pushed a commit that referenced this pull request Mar 3, 2018

Merge pull request #2655 from samvera/rubocop

a8a668c

Update RuboCop support; use Bixby style guide

Update RuboCop support; use Bixby style guide #2655

Update RuboCop support; use Bixby style guide #2655

Conversation

no-reply commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrochkind Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne commented Feb 16, 2018

no-reply commented Feb 16, 2018

jcoyne commented Feb 16, 2018

no-reply commented Feb 16, 2018

jcoyne commented Feb 16, 2018

no-reply commented Feb 16, 2018

no-reply commented Feb 16, 2018

no-reply commented Feb 16, 2018

laritakr commented Feb 16, 2018 • edited Loading

no-reply commented Feb 19, 2018

jenlindner commented Feb 22, 2018

jcoyne commented Feb 22, 2018

jenlindner commented Feb 22, 2018 • edited Loading

no-reply commented Feb 22, 2018 • edited Loading

cjcolvar commented Feb 23, 2018

no-reply commented Feb 26, 2018

jenlindner commented Feb 26, 2018

cjcolvar left a comment

Choose a reason for hiding this comment

jrochkind Feb 21, 2018 •

edited

Loading

laritakr commented Feb 16, 2018 •

edited

Loading

jenlindner commented Feb 22, 2018 •

edited

Loading

no-reply commented Feb 22, 2018 •

edited

Loading