Let RSpec/SpecFilePathFormat leverage ActiveSupport inflections when configured

[corsonknowles]

Fix #740

Credit to this PR, reviving with performance and configuration comments addressed:

• https://github.com/rubocop/rubocop-rspec/pull/1266/files#r862618003

---

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

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.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.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[corsonknowles]

bundle exec rake is failing on the base branch, unless I am missing something

edit: I was missing the need to bundle update to get a clean run

[corsonknowles]

Alternatively: We could remove all the handling to cache the lookup by rescuing and warning on a load error when the user has configured to use Inflectors but they aren't available.

The current approach fails gracefully when configured on, but unavailable.

[corsonknowles]

most should never need to set this, but just in case

[corsonknowles]

@bquorning I think I addressed all the comments on the original proposed fix PR.

We tested this locally in one of our repos with good results.

I made the path configurable, but I didn't think an array of possible integrations in config was warranted at this point.

In the event that someone does come along and adds an adapter to a new Inflector library, they could add a new top level config key and use the same InflectorPath key.

I guess I just think it's unlikely we'll reach more than 3 custom inflectors at which point we could always switch to making users pick a style instead of having a boolean option.

[bquorning]

I am not saying that we should support more inflectors, but just in case: Should we rename this to e.g. Inflector: (default|active_support)? (And a SupportedInflectors?) Or would that be premature?

[corsonknowles]

I had tried to explain this choice in a PR comment:

I made the path configurable, but I didn't think an array of possible integrations in config was warranted at this point.
In the event that someone does come along and adds an adapter to a new Inflector library, they could add a new top level config key and use the same InflectorPath key.
I guess I just think it's unlikely we'll reach more than 3 custom inflectors at which point we could always switch to making users pick a style instead of having a boolean option.

Happy to change it if you want!

But to reframe the above, I was on the "premature" side as I was implementing this. I thought it would be more user-friendly to use a boolean than have folks look up two "magic strings" to use this. They need both the key and the value if we abstract it, whereas they just need the key if it's a boolean.

Just let me know if you want it though.

[bquorning]

I am not crazy about this method. It feels like maybe we are memoizing on the wrong level somehow.

[corsonknowles]

I kind of thought we should hoist this (the logic is in the module still), so we can use it more transparently.

But open to alternatives...

[bquorning]

I am not sure if this method should memoize without considering cop_config.

[corsonknowles]

I think I need the config, but I will see what I can do

[corsonknowles]

I mean even if I separate the availability from the config check, I still need the config to look up the inflector_path and only in the case where it is available? or something like it.

So config is somewhat coupled to availability

[corsonknowles]

Best I can think of would be something like this, but that seems worse to me:

        def active_support_available?
          return false unless cop_config.fetch(
            'UseActiveSupportInflections',
            false
          )

          ActiveSupportInflector.available?(cop_config)
        end

        def inflector
          @inflector ||= if active_support_available?
                           ActiveSupportInflector
                         else
                           DefaultInflector
                         end
        end

Let me know whart you're aiming for here

[bquorning]

Thinking this though once more, I happened to ask myself this question: Do we expect people to give different values for InflectorPath in different configuration files? Would that even work? I think ActiveSupport::Inflector sets its inflections globally.

So, should InflectorPath really be a global configuration that cannot be overwritten?

[corsonknowles]

I would guess the thinking here is that someone using ActiveSupport but not Rails might have a customized directory structure. And monkey patchers could still use this.

ActiveSupport::Inflector does seem pretty locked down against changes to the base file, and I imagine the same reasoning applies to not moving the config file around, so Rails itself should be pretty stable around which path it scaffolds the file to.

Maybe something else with nested Rails apps or nested Rails Engines?

I do think this is mildly superfluous, but it was one of the comment requests on the original PR, and now that we have it, I'm hesitant to rip it out again since in theory we're making life easier for +1% of users.

IMO, the heavyweight thing is we have to require 2 files and fail gracefully. Path configurability is minor compared that that.

[corsonknowles]

If you think its worth the sacrifice, we could take the path out of config and then make it a constant on the cop class.

I'm imagining somewhere someone will eventually have to type ruby -e and patch our cop as they run it from the command line to get it working for their use case, but perhaps the tradeoff is worth it?