collectiveidea / interactor

Interactor provides a common interface for performing complex user interactions.
MIT License
3.36k stars 212 forks source link

add rubocop and drop support for Rubies < 2.0 #124

Closed hedgesky closed 7 years ago

hedgesky commented 7 years ago

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

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:

laserlemon commented 7 years ago

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

hedgesky commented 7 years ago

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 commented 7 years ago

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

laserlemon commented 7 years ago

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 commented 7 years ago

Changes in last commit:

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

hedgesky commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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.