add rubocop and drop support for Rubies < 2.0 #124

hedgesky · 2017-03-22T10:09:18Z

This PR consists of two commits. First one introduces following changes:

Adds rubocop as a dev dependency (use conservative version locking as official manual advises;
Adds .rubocop.yml config;
Fixes some style issues;
Changes default rake task;
Updates travis.yml to run rake instead of rspec

When first commit failed Travis checks I realized that fresh Rubocop requires only Ruby >= 2.0. Keeping in mind that there is a plan to drop support for Rubies < 2.1 in Interactor 4.0, I've decided to update this PR accordingly.

So the second commit does following:

removes Rubies 1.9.3, 2.0 from Travis config;
sets Rubocop target Ruby version to 2.1;
add required_ruby_version = ">= 2.1" in gemspec.

laserlemon

Thank you so much for this! I didn't expect help on 4.0 so quickly! Most of this looks great; I just had a few questions. Also, could you please rebase this against the very little pending work I've done in the v4 branch? Thank you!

laserlemon · 2017-03-22T10:38:44Z

spec/integration_spec.rb

@@ -25,7 +25,8 @@ def build_organizer(options = {}, &block)
  #  └─ interactor5

  let(:organizer) {
-    build_organizer(organize: [organizer2, interactor3, organizer4, interactor5]) do
+    interactors = [organizer2, interactor3, organizer4, interactor5]
+    build_organizer(organize: interactors) do


Are this and the similar changes below just a line length concern?

Yep. Both ways have their benefits and downsides. Introducing intermediate variable seems a bit clumsy, but it keeps lines short. Short lines are easier to perceive and they are two-panes-friendly:

Anyway, it's not a big issue if concentrated in a single file.

Yeah, I'm all for the 80 character limit but I typically relax that rule a bit for specs. But maybe I shouldn't!

laserlemon · 2017-03-22T10:40:56Z

spec/integration_spec.rb

          raise "foo"
-          context.steps << :around_before
-          interactor.call
-          context.steps << :around_after


Although it may seem obvious to Rubocop, there's some value in the assertion that these lines are not called, even after a raise. This applies to several of the changes below as well.

I'm open to discussion on this point!

Yeah, I see your point. I thought about smth like this:

def unexpected_error! raise "foo" end

It may be more intention-revealing (the "unexpected" nature), but plain raise is far simpler and understable. What do you think?

I like your use of unexpected_error! although we might have to define that method within the interactor's definition in the spec rather than as a helper method in the spec, which muddies the waters a little bit.

laserlemon · 2017-03-22T10:45:14Z

spec/support/lint.rb

-      expect(Interactor::Context).to receive(:build).once.with(foo: "bar") { context }
+      expect(
+        Interactor::Context
+      ).to receive(:build).once.with(foo: "bar") { context }


If this is a line length concern, would Rubocop accept this style:

expect(Interactor::Context).to receive(:build) .once.with(foo: "bar") { context }

laserlemon · 2017-03-22T10:49:34Z

interactor.gemspec

@@ -1,19 +1,24 @@
 # encoding: utf-8
+require "English"


Thank you, I didn't know about this!

laserlemon · 2017-03-22T10:53:48Z

interactor.gemspec

  spec.test_files = spec.files.grep(/^spec/)

+  spec.required_ruby_version = ">= 2.1"


👏 🏆 💐 🥇

laserlemon · 2017-03-22T10:54:58Z

interactor.gemspec

  spec.add_development_dependency "bundler", "~> 1.7"
  spec.add_development_dependency "rake", "~> 10.3"
+  spec.add_development_dependency "rubocop", "~> 0.47.1"


I'd rather put this dependency in the Gemfile and keep the gemspec dependencies to only bundler and rake.

laserlemon · 2017-03-22T10:56:33Z

lib/interactor.rb

@@ -153,14 +153,12 @@ def run!
  # each interactor class.
  #
  # Returns nothing.
-  def call
-  end
+  def call; end


I'm not a big fan of this, but I suppose if the community has spoken! 😄

Rubocop default settings are easily adjustable to project's needs. I'll remove this change.

laserlemon · 2017-03-22T10:59:28Z

Actually, this may need to be reopened as a PR against the v4 branch as well. 😕

hedgesky · 2017-03-22T12:42:48Z

I've added replies to your comments; some of them require further discussion. I'll close this PR and reopen a new one on v4 branch.

Thank you for a quick response 😄

hedgesky · 2017-03-22T12:48:42Z

Actually, it's possible to just change base branch via github UI. Closed this too rashly 😄

laserlemon · 2017-03-22T15:55:14Z

I used the GitHub web UI to resolve conflicts with the v4 branch. I may have inadvertently removed the Ruby version restriction from the gemspec, but on second thought, I'm not sure that we want it. While we're dropping support for Rubies < 2.1, it's possible that Interactor 4.0 could work with Ruby 2.0, depending on what Ruby features we take advantage of. Dropping support for Ruby 2.0 is more of a statement that you can try to use Interactor with Ruby 2.0 but do so at your own risk. I'm open to discussion on this point though. It's certainly an option to draw a harder line with our Ruby version restriction in the gemspec.

hedgesky · 2017-03-23T10:02:33Z

Changes in last commit:

I've decided to keep 80 character limit in integration_spec;
Added #unexpected_error! in integration_spec;
Changed spec/support/lint.rb:46 following your request;
Set spec.required_ruby_version = ">= 2.0";
Moved rubocop from gemspec to Gemfile;
Revert change with one-lined empty defs in lib/interactor.

About Ruby versions: support for required keyword arguments appeared only in 2.1. I think we'll definitely need them if we're going to implement #123.

laserlemon · 2017-03-23T11:22:19Z

@hedgesky Your changes look really good. You said in your comment that keyword arguments appeared in Ruby 2.1, but I'm seeing otherwise. You're absolutely right though that keyword arguments will most likely be required so that would be the right reason to scope the Ruby version in the gemspec. It seems like we'll need to scope it to >= 2.0 and then only officially support non-EOL Rubies, which would be >= 2.1 as of this writing. Does that sound right to you?

hedgesky · 2017-03-23T11:53:42Z

Yeah, keyword arguments appeared in 2.0, but I told about required ones:

def foo(bar:)
  puts bar
end

They appeared in 2.1. Both your link and official changelog on Ruby github mentions that.
Most likely we'll need both required and non-required arguments, so maybe 2.0 support doesn't make sense in our case. Your thoughts?

laserlemon · 2017-03-23T11:59:33Z

It could be that even though required keyword arguments would be a big win for Interactor 4, they might not be strictly necessary because the structure of the keyword arguments is up to the developer when they define the call instance method. Plus, I imagine defining keyword arguments at all will be completely optional. There may be some difference between Rubies 2.0 and 2.1 as far as how we determine the nature of what keyword arguments are being requested so we can extract those from the context and send them along.

hedgesky · 2017-03-23T13:18:33Z

I suggest delaying the decision about 2.0/2.1 until we have more information about Interactor 4 implementation details. For now we can just stick with 2.0.
Is it OK with you?

laserlemon · 2017-03-23T13:23:18Z

@hedgesky Yes, that sounds fine, but let's make one change then. If we'll target Ruby 2.0 for now, let's add 2.0 back to .travis.yml but allow it to fail, like we do for ruby-head, since it's EOL.

laserlemon · 2017-03-23T13:34:41Z

@hedgesky Looks good. Is there anything else you want to add before I merge?

hedgesky · 2017-03-23T13:41:14Z

Nope, that's all. Go right ahead 😄
Thanks for the discussion, it was really productive! If there is anything else I can help with, feel free to reach out to me. I'll be glad to speed up Interactor 4 release ✈️

laserlemon · 2017-03-23T13:42:49Z

@hedgesky Absolutely, I hope you don't feel like you're off the hook now! 😄 You've been a big help already and I know I'll need help to get 4.0 out the door. 🚀

hedgesky · 2017-03-23T13:59:24Z

Looks like Drop support for Rubies < 2.1 and Add and configure Rubocop to ensure consistent code style project cards can be moved to Done section now (really cool github feature, from my point of view).

I'm going to work on raise -> throw switch following #120. Would you mind if I reopen it? Probably it deserves v4.0.0 milestone, too.

add rubocop

f9f05b3

hedgesky force-pushed the add_rubocop branch 2 times, most recently from 1d785e0 to ccc9b89 Compare March 22, 2017 10:25

drop support for Rubies < 2.1.0

0af010d

hedgesky force-pushed the add_rubocop branch from ccc9b89 to 0af010d Compare March 22, 2017 10:27

hedgesky changed the title ~~add rubocop~~ add rubocop and drop support for Rubies < 2.1 Mar 22, 2017

laserlemon requested changes Mar 22, 2017

View reviewed changes

laserlemon added this to the v4.0.0 milestone Mar 22, 2017

laserlemon assigned hedgesky Mar 22, 2017

hedgesky closed this Mar 22, 2017

hedgesky changed the base branch from master to v4 March 22, 2017 12:46

hedgesky reopened this Mar 22, 2017

Merge branch 'v4' into add_rubocop

80e354e

implemented requested changes (collectiveidea#124)

04d591c

configure rubocop to Ruby 2.0 syntax

d4f178a

laserlemon added the feature label Mar 23, 2017

hedgesky changed the title ~~add rubocop and drop support for Rubies < 2.1~~ add rubocop and drop support for Rubies < 2.0 Mar 23, 2017

return Ruby 2.0 to Travis config

494e672

laserlemon approved these changes Mar 23, 2017

View reviewed changes

laserlemon merged commit 6de0c22 into collectiveidea:v4 Mar 23, 2017

hedgesky deleted the add_rubocop branch March 23, 2017 14:12

laserlemon pushed a commit that referenced this pull request Mar 31, 2017

implemented requested changes (#124)

e9a30dc

taylorthurlow pushed a commit to taylorthurlow/interactor that referenced this pull request Jul 12, 2020

implemented requested changes (collectiveidea#124)

0ffa4e9

hedgesky removed their assignment Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rubocop and drop support for Rubies < 2.0 #124

add rubocop and drop support for Rubies < 2.0 #124

hedgesky commented Mar 22, 2017 •

edited

Loading

laserlemon left a comment

laserlemon Mar 22, 2017

hedgesky Mar 22, 2017 •

edited

Loading

laserlemon Mar 22, 2017

laserlemon Mar 22, 2017

laserlemon Mar 22, 2017

hedgesky Mar 22, 2017

laserlemon Mar 22, 2017

laserlemon Mar 22, 2017

hedgesky Mar 22, 2017

laserlemon Mar 22, 2017

laserlemon Mar 22, 2017

laserlemon Mar 22, 2017

laserlemon Mar 22, 2017

hedgesky Mar 22, 2017

laserlemon commented Mar 22, 2017

hedgesky commented Mar 22, 2017 •

edited

Loading

hedgesky commented Mar 22, 2017

laserlemon commented Mar 22, 2017

hedgesky commented Mar 23, 2017

laserlemon commented Mar 23, 2017 •

edited

Loading

hedgesky commented Mar 23, 2017 •

edited

Loading

laserlemon commented Mar 23, 2017

hedgesky commented Mar 23, 2017

laserlemon commented Mar 23, 2017

laserlemon commented Mar 23, 2017

hedgesky commented Mar 23, 2017

laserlemon commented Mar 23, 2017

hedgesky commented Mar 23, 2017

		spec.test_files = spec.files.grep(/^spec/)

		spec.required_ruby_version = ">= 2.1"

add rubocop and drop support for Rubies < 2.0 #124

add rubocop and drop support for Rubies < 2.0 #124

Conversation

hedgesky commented Mar 22, 2017 • edited Loading

laserlemon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedgesky Mar 22, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laserlemon commented Mar 22, 2017

hedgesky commented Mar 22, 2017 • edited Loading

hedgesky commented Mar 22, 2017

laserlemon commented Mar 22, 2017

hedgesky commented Mar 23, 2017

laserlemon commented Mar 23, 2017 • edited Loading

hedgesky commented Mar 23, 2017 • edited Loading

laserlemon commented Mar 23, 2017

hedgesky commented Mar 23, 2017

laserlemon commented Mar 23, 2017

laserlemon commented Mar 23, 2017

hedgesky commented Mar 23, 2017

laserlemon commented Mar 23, 2017

hedgesky commented Mar 23, 2017

hedgesky commented Mar 22, 2017 •

edited

Loading

hedgesky Mar 22, 2017 •

edited

Loading

hedgesky commented Mar 22, 2017 •

edited

Loading

laserlemon commented Mar 23, 2017 •

edited

Loading

hedgesky commented Mar 23, 2017 •

edited

Loading