Add mixins folder #942

mockdeep · 2020-06-19T19:22:54Z

This moves several files to a new mixins/ folder. I was going to add
some new macros, per this comment, but thought it might clutter
things up a bit to put them in the root folder. I thought of adding a
macros/ folder, but decided to emulate the rubocop source
instead.

Before submitting the PR make sure the following are checked:

Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
~~Added tests.~~
~~Updated documentation.~~
~~Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.~~
The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

pirj

Thanks for adding some structure!

pirj · 2020-06-20T12:13:30Z

lib/rubocop-rspec.rb

+require_relative 'rubocop/rspec/mixin/top_level_group'
+require_relative 'rubocop/rspec/mixin/variable'
+require_relative 'rubocop/rspec/mixin/final_end_location'
+require_relative 'rubocop/rspec/mixin/blank_line_separation'


I guess maybe rubocop/rspec/cop/mixin/* if they are intended to be included in cops like RuboCop does it?

@pirj yeah, I thought about that briefly. The weird thing in our case is that FinalEndLocation gets included in two places, one being lib/rubocop/rspec/corrector/move_node.rb. Should I put that in the cop/mixin/ directory anyway, or leave it in rubocop/rspec/? It also gets included in blank_line_separation.rb which is itself included in cops.

@pirj I went ahead and moved them over to the cop/rspec/ folder. That one case is a little odd, but I think it's probably still better this way.

mockdeep · 2020-06-27T19:47:37Z

Hmm, is this failure related to something on master?

bquorning

Don’t worry about failures on edge rubocop; I think the latest master has some quite significant changes that we need to address separately.

Darhazer · 2020-06-29T16:15:37Z

lib/rubocop-rspec.rb

 require_relative 'rubocop/rspec/wording'
 require_relative 'rubocop/rspec/language'
 require_relative 'rubocop/rspec/language/node_pattern'
-require_relative 'rubocop/rspec/top_level_group'
+
+require_relative 'rubocop/cop/rspec/mixin/align_let_brace'


Technically, this is not a mixin. It's a helper class used by 2 cops.

Agree. Let's keep in rubocop/rspec.
Everything else is 👍

Okay, sorry for the delay on this. I've moved align_let_brace back to where it came from.

pirj

Thanks!

pirj · 2020-07-01T11:11:15Z

lib/rubocop-rspec.rb

 require_relative 'rubocop/rspec/wording'
 require_relative 'rubocop/rspec/language'
 require_relative 'rubocop/rspec/language/node_pattern'
-require_relative 'rubocop/rspec/top_level_group'
+
+require_relative 'rubocop/cop/rspec/mixin/align_let_brace'


Agree. Let's keep in rubocop/rspec.
Everything else is 👍

mockdeep · 2020-07-07T22:03:00Z

Not sure why the build is failing on this. It works for me locally.

bquorning · 2020-07-08T14:09:54Z

Not sure why the build is failing on this. It works for me locally.

I think you’re blocked by #955.

pirj · 2020-07-10T08:04:47Z

#955 is merged. Please rebase. Sorry for the trouble.

bquorning · 2020-07-10T19:32:06Z

lib/rubocop-rspec.rb

 require_relative 'rubocop/rspec/wording'
 require_relative 'rubocop/rspec/language'
 require_relative 'rubocop/rspec/language/node_pattern'
-require_relative 'rubocop/rspec/top_level_group'
+
+require_relative 'rubocop/cop/rspec/mixin/top_level_describe'


When we change the folder like this, shouldn’t we also change the namespace? E.g. RuboCop::RSpec::TopLevelDescribe -> RuboCop::Cop::RSpec::TopLevelDescribe.

Agree.

@mockdeep We've just renamed RuboCop::Cop::RSpec::Cop to RuboCop::Cop::RSpec::Base following RuboCop core's naming.
If we happen to move classes/modules around namespaces, and if it happens those are used by custom cops, we'll break them with a minor version update.
So for Base, we've decided to keep the old class, Cop as well until 2.0.
I don't see how it would be possible to release this change with namespace before we release 2.0 without similar breakage risks.

AFAI understand #863 depends on this pull request. But if this is going to be released in 2.0, that means that #863 will only be released as enabled in 3.0, until then it will be pending.

So I propose two options for #863:

untangle it from this pull request, and release separately

keep it dependent on this pull request and release post-2.0

Please let me know what you think.

This moves several files to a new `mixins/` folder. I was going to add some new macros, per [this comment][1], but thought it might clutter things up a bit to put them in the root folder. I thought of adding a `macros/` folder, but decided to emulate the [rubocop source][2] instead. [1]: rubocop#934 (comment) [2]: https://github.com/rubocop-hq/rubocop/tree/master/lib/rubocop/cop/mixin

pirj · 2020-07-15T11:37:26Z

Adjusted module namespace and rebased.
Let's wait for Github to catch up.

bquorning

Is this enough of a breaking change to wait for 2.0? Can we just alias the modules like we did for Cop?

pirj · 2020-07-15T23:28:30Z

That's an option. Depends on how essential this is to move #863 forward.

mockdeep · 2020-07-16T00:04:06Z

Unfortunately, as you may have noticed, I haven't been able to be very consistent on following up with these PRs. I don't think #863 depends on this, since, if I remember correctly it was built on #934 instead. I was picturing adding some more macros and using those instead, but we could stick with #863 as is if you want to get that out sooner.

pirj · 2020-07-16T08:53:08Z

No rush, take your time and make any additions you see fit in #863.
We'll take care of this pull request later.
Even though RuboCop 1.0 and RuboCop RSpec 2.0 are around the corner, there's a fat list of leftovers there and something to work on here as well, so I guess it's not going to happen earlier than in 3-4 weeks.

pirj · 2020-10-22T18:22:01Z

I'll take care of rebasing this.
This is optional, but a nice cleanup nevertheless.

pirj · 2020-10-22T21:28:14Z

Superseded by #1056
Thanks, @mockdeep, this will become part of the 2.0 release!

mockdeep · 2020-10-22T21:34:04Z

@pirj thank you for rebasing!

mockdeep force-pushed the rf-mixins branch from 6928359 to 435fbb5 Compare June 19, 2020 19:26

pirj approved these changes Jun 20, 2020

View reviewed changes

mockdeep force-pushed the rf-mixins branch from 435fbb5 to 963223f Compare June 27, 2020 17:55

pirj requested review from bquorning and Darhazer June 29, 2020 07:35

bquorning approved these changes Jun 29, 2020

View reviewed changes

Darhazer reviewed Jun 29, 2020

View reviewed changes

pirj approved these changes Jul 1, 2020

View reviewed changes

mockdeep force-pushed the rf-mixins branch 2 times, most recently from 9544e42 to be285e9 Compare July 7, 2020 21:45

bquorning reviewed Jul 10, 2020

View reviewed changes

pirj added this to the 2.0 milestone Jul 15, 2020

pirj force-pushed the rf-mixins branch from f7b4197 to bfb57f0 Compare July 15, 2020 13:31

bquorning approved these changes Jul 15, 2020

View reviewed changes

pirj changed the base branch from master to release-2.0 October 22, 2020 18:22

This was referenced Oct 22, 2020

Release 2.0 #1055

Merged

Add mixins folder #1056

Merged

pirj closed this Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mixins folder #942

Add mixins folder #942

mockdeep commented Jun 19, 2020

pirj left a comment

pirj Jun 20, 2020

mockdeep Jun 20, 2020

mockdeep Jun 27, 2020

mockdeep commented Jun 27, 2020

bquorning left a comment

Darhazer Jun 29, 2020

pirj Jul 1, 2020

mockdeep Jul 7, 2020

pirj left a comment

pirj Jul 1, 2020

mockdeep commented Jul 7, 2020

bquorning commented Jul 8, 2020

pirj commented Jul 10, 2020

bquorning Jul 10, 2020

pirj Jul 15, 2020

pirj commented Jul 15, 2020

bquorning left a comment

pirj commented Jul 15, 2020

mockdeep commented Jul 16, 2020

pirj commented Jul 16, 2020

pirj commented Oct 22, 2020

pirj commented Oct 22, 2020

mockdeep commented Oct 22, 2020

Add mixins folder #942

Add mixins folder #942

Conversation

mockdeep commented Jun 19, 2020

pirj left a 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

mockdeep commented Jun 27, 2020

bquorning left a 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

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockdeep commented Jul 7, 2020

bquorning commented Jul 8, 2020

pirj commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jul 15, 2020

bquorning left a comment

Choose a reason for hiding this comment

pirj commented Jul 15, 2020

mockdeep commented Jul 16, 2020

pirj commented Jul 16, 2020

pirj commented Oct 22, 2020

pirj commented Oct 22, 2020

mockdeep commented Oct 22, 2020