Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Avoid repetitive definitions of CustomTransform in RSpec/FilePath cop #740

Open
aeroastro opened this issue Feb 12, 2019 · 2 comments · May be fixed by #1266
Open

Proposal: Avoid repetitive definitions of CustomTransform in RSpec/FilePath cop #740

aeroastro opened this issue Feb 12, 2019 · 2 comments · May be fixed by #1266

Comments

@aeroastro
Copy link

Summary

I would like to achieve the following 2 things for RSpec/FilePath.
I am willing to implement the solution when we reach an agreement on this issue.

Avoid repetitive CustomTransform definitions for single inflection.

When using ActiveSupport::Inflector, we have to write only 1 definition for 1 word with special inflection. On the other hand, when using rubocop-rspec, we have to define multiple definitions (all the composite words derived from a special word). This makes development a little bit less productive.

For example, let's assume that we use a special word like PvP in a project.

When using ActiveSupport::Inflector, all we have to do is write a single rule on PvP.

ActiveSupport::Inflector.inflections(:en) do |inflect|
  inflect.acronym 'PvP'
end

When we use the current CustomTransform, we have to define all the transforms as follows.

RSpec/FilePath:
  CustomTransform:
    PvP: pvp
    SyncPvP: sync_pvp
    AsyncPvP: async_pvp
    PvPOverPvP: pvp_over_pvp
    PvPsController: pvps_controller

This is mainly because the exact-match substitution is handled before snake casing.

https://github.com/rubocop-hq/rubocop-rspec/blob/d69442f17d874cff8f5c11faabd929534c8fc717/lib/rubocop/cop/rspec/file_path.rb#L83-L89

I would like to let the following config just work as ActiveSupport::Inflector config does.

RSpec/FilePath:
  CustomTransform:
    PvP: pvp

Share the same settings as ActiveSupport::Inflector has

I am not so inclined to achieve this as the first one, but, hopefully, I would like to share the same config as we have in Rails application. This motivation is not applicable to non-Rails user. Therefore, I would like to use less-invasive way to achieve this.

In our project, we are using require directive in config YAML to require config/initializers/inflections.rb, where ActiveSupport::Inflector config exists, but there may be a more sophisticated intuitive way.

How to solve this issue

IMHO, I believe that ActiveSupport::Inflector, which is famous for its sophisticated string handling, can solve this issue. Since this idea has both pros and cons, I have set up this issue.

https://api.rubyonrails.org/classes/ActiveSupport/Inflector.html

If RuboCop policy prioritize less dependencies over reinventing the wheel, we can just partially import the code from ActiveSupport::Inflector

Pros and Cons

  • Pros
    • Avoiding reinventing the wheel on string inflections.
    • Reduced code complexity on current and future code.
  • Cons
    • Breaking backward compatibilities a little.
    • Increased dependencies of RuboCop::RSpec on ActiveSupport::Inflector.

Other Information

I made up a gem rubocop-inflector to solve this issue. However, since the gem touch the internal method of rubocop-rspec, later I have found out it is fragile and not so a recommended way to solve the issue. Although it is very thin and comparatively easy to maintain.

https://github.com/aeroastro/rubocop-inflector

@aeroastro aeroastro changed the title Proposal: Avoid repetitive definitions of CustomTransform in RSpec/FilePath Proposal: Avoid repetitive definitions of CustomTransform in RSpec/FilePath cop Feb 12, 2019
@dgollahon
Copy link
Contributor

Hi @aeroastro, thanks for the issue. I recently ran across a similar problem in one of my repos as well.

I think this is worth considering, but we should definitely not have a hard dependency on ActiveSupport::Inflector. I'm also not particularly wild about the idea of vendoring the AS code (unless it turns out to be much smaller/simpler than I'm picturing), but one thing we could consider is try to check the AS inflections if ActiveSupport::Inflector is defined and then fall back to our existing transforms/logic, or something like that.

This requires some thought. cc: @bquorning @Darhazer

@pirj
Copy link
Member

pirj commented May 10, 2019

Since the gem works fine, we could recommend it as an optional dependency for this specific case.

To circumvent the brittleness, can you provide more information on what internal API you use in it? Probably it would be possible to expose some stable interface so that the behaviour can be reliably enhanced by optionally including rubocop-inflector?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants