WIP: Rspec aliases

[sl4vr]

This is an attempt of implementation of #333. Allowing to configure 3rd partly DSLs as valid RSpec syntax.

Suggested configuration:

AllCops:
  RSpec:
    Aliases:
      Hooks:
      - custom_hook
      Examples:
	    Regular:
        - custom_it
        Focused:
        - fcustom_it

The problem is that if we need to be able to consider configuration regarding to every file - we need somehow build dynamic matchers for hooks, example_groups, etc.

AFAICU def_node_matcher defines matcher in class on load, so we cannot mutate it during runtime (at least it looks complicated), so instead I tried to rewrite some matchers to be more common, and then inside them I check was it, for example, hook or not.

Now I want to understand is this approach is viable or should I look into some other options?

---

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 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).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.

[sl4vr]

Another implementation is here

Probably it's a really edge case when you need to specify different aliases in different directories. I would even say no one would do that. So maybe it worths just to go with top level config for aliases. In this case all the patterns may be kept unchanged, so only Language module will require a bit of rework.

[pirj]

Probably it's a really edge case when you need to specify different aliases in different directories. I would even say no one would do that.

Agree 👍

[pirj]

Good headstart!

[sl4vr]

Thank you, @pirj!

I've made several proofs of concepts, but decided not to open multiple PRs against this repository's master and just refer PRs here instead.

Rspec.Aliases in root-level config

sl4vr#2

Pros

• No need to change anything but RuboCop::RSpec::Language module
• Should be faster then dynamic matchers since it fully uses of AST matchers

Cons

On the other hand it's pretty inconsistent:

• Bottom-level configuration for aliases will be ignored in most cases
• In some rare cases root-level aliases can be ignored. E.g. if project doesn't use bundler and we run rubocop from some bottom-level directory which already has its own .rubocop.yml without Rspec.Aliases. But still no one gonna do that.

Caching

If bottom-level directories have .rubocop.yml and caching is enabled then changing of hooks in top-level config will be ignored. Need to figure out how to handle it properly.

Testing

config context won't work here because aliases configuration is loaded on gem loading and doesn't fully use rubocop config loading mechanism. I think instead it's enough to test just aliases configuration loading though.

rspec_language directive in root-level config

sl4vr#3

The main difference is that directive is not treated as regular setting by user, so user probably won't expect it will be accessible in bottom-level configurations.
Unfortunately it has pretty same problems as above implementation.

.rubocop-rspec config file

sl4vr#1

Uses .rubocop-rspec.yml to define RSpec DSL. It creates .rubocop-rspec.yml filled with defaults on first run now.
Can be reworked to merge with default config instead if it's better to allow user to just extend RSpec DSL, but not redefine.
I personally like explicit version because it's obvious and almost doesn't require any documentation.

Looks like more consistent solution since all the language configuration is placed in separate yaml file and not associated with rubocop configs at all.

There are some cons still:

• It introduces yet another config file
• It doesn't work with caching at all. Some research is needed to be done here if we'll decide to go with this one.

[pirj]

Sorry again for taking so long to review.

Wow, that is impressive! I really like them all, let's discuss which approach to take.

There's a generic problem I've noticed while playing around with this thing, not related to your code, but to how RuboCop works. We can't load the config using regular RuboCop's mechanisms before our classes are fully loaded. Config validation is an inherent part of its config loading, along with checking if cops configured in the config do exist.
This makes me think there's no way for us to use RuboCop's config on that stage.

Another thing to keep in mind is that there are multiple ways how config is defined, it can be a ~/.rubocop.yml, or a config common for several projects in a shared directory. Also, there are command-line options that affect config loading.

It's definitely not a trivial task to keep up with all of that.

2. Rspec.Aliases in root-level config sl4vr/rubocop-rspec#2

I really like how the config file is structured. It's a regular RuboCop config file. This will work fine for a typical RuboCop installation. This solution is so simple and elegant I'm inclined to go with this without even looking at the other options.
Yes, it's limited to a .rubocop.yml in the project root. But it's a good start that will solve several issues.

it's enough to test just aliases configuration loading though

Agree 👍

One thing that makes me think is that we're putting the responsibility of keeping their .rubocop.yml in sync with the Gemfile, e.g. if Pundit is installed, the user has to add role to example group aliases. My idea was to push that responsibility to the gems, e.g. add this configuration section in Pundit's config/default.yml.
This can be made possible with the help of inherit_mode:

The optional directive inherit_mode specifies which configuration keys that have array values should be merged together instead of overriding the inherited value.

However, it's not a showstopper, as it will take a while to spread the word about those new configuration options. I believe we'll have to add a "config_loaded" callback to RuboCop or lazily initialize our SelectorSets (with a lambda instead of a +). We can postpone this decision.

3. rspec_language directive in root-level config sl4vr/rubocop-rspec#3

rspec_language might go under AllCops/RSpec, right?
I like this even more.
Keeping in mind RSpec 4 is just around the corner and there might be some DSL changes, we could get rid of hardcoding it in Language and take what we need from the config depending on some TargetRSpecVersion setting just like rubocop-rails does with TargetRailsVersion.
Funny enough, this reminds me of the XML days :D

1. .rubocop-rspec config file sl4vr/rubocop-rspec#1

Seems similar to 3), but it implies that an additional config file is used. This limits us in the ability to push the responsibility to Pundit and other gems.

---

Hope you can grab something from my random thoughts.
I have no intention of telling you how to proceed unless you insist on that. You went too far already to be able to figure out the best solution on your own.

Please don't hesitate to share your thoughts on early stages here or on the ticket.

[Darhazer]

A couple of minor implementation notes.

When it comes to the Language module, we don't have any complex patterns, we mostly check the method name. So we can easily replace those with dynamic checks on the name, and checking for the node type (if needed). I was even thinking on extending it to provide on_example_group, on_hook, etc., like we have on_top_level_describe, but didn't find enough cops that would benefit (in terms of code simplification) from such a change. But with the customizable language, it may yield results.

[Darhazer]

Do we really need a matcher to tell that a block is empty?
add_offense(hook) is node.body.nil? should do the job

[Darhazer]

I don't think you need a pattern for this.

return false unless node.block_type?
hooks.include?(node.method_name)

[pirj]

Brilliant idea @Darhazer, as always. I blame myself for overseeing such an obvious approach.

@sl4vr With such an approach the load order (classes first, and config only after) is not an issue anymore. I hope you can adopt this and come up with a solution that is both clean and flexible.

[sl4vr]

Amazing! I'll try to look into that way definitely.

[pirj]

@sl4vr Do you need any help here?

[sl4vr]

@pirj Not yet, just trying to find time to proceed. Thanks for help offering, I'll report here if I'll face any new difficulties.

[pirj]

Closing in favour of #956