ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.17k stars 404 forks source link

Use component path for generating RSpec files #2005

Closed neanias closed 2 months ago

neanias commented 2 months ago

What are you trying to accomplish?

Generally, the path to an RSpec file should match the app path, but substituting app for spec. If a user changes the generation path for their components, then the generator should use an appropriate path that matches that. In the example where the user changes the generation path from app/components to app/views/components, the RSpec component spec should be created in spec/views/components.

What approach did you choose and why?

The logic chosen here checks that the component is still being generated into the app dir, otherwise it will just fallback on the usual destination of spec/components. I've used simple string matching of start_with?("app#{File::SEPARATOR}"), but maybe using something in the File or Pathname classes would be more flexible.

Anything you want to highlight for special attention from reviewers?

Since this includes the AbstractGenerator, we can remove the file_name method as it's provided by the module.

(Hi, Simon!)

neanias commented 2 months ago

Tests probably aren't a bad idea — I'll get one run up. Good question re breaking change. I suppose it is since it's actively changing generator behaviour... Rails allows overrides of builtin generators, so I can write some documentation about how to set it back to the old way? I can also expand on the changelog entry. Lmk what you think!

joelhawksley commented 2 months ago

@neanias since it's a breaking change, we'll need to either hold it for the next major release or put it behind a config flag. I would vote we do the latter, adding a config flag that defaults to false for the new behavior. We can then remove the flag in the next major release!

neanias commented 2 months ago

@joelhawksley Sounds like a plan. Should I put it under the generate collection of config options?

neanias commented 2 months ago

The config option feels like a bit of a mouthful, but I think it's clear enough?

boardfish commented 2 months ago

👋 Nice to see you around here, @neanias! And thanks for a very thorough PR 💯

boardfish commented 2 months ago

Also, worth verifying if we need the same for Minitest.

neanias commented 2 months ago

Just noticed I've used "you" in my documentation for the new config flag. Is that ok or should I change it?

joelhawksley commented 2 months ago

Just noticed I've used "you" in my documentation for the new config flag. Is that ok or should I change it?

I'd prefer to avoid "you". Mind making the change? I'll review the language in this PR for ya ❤️

neanias commented 2 months ago

Ok, I think that's everything? Re the potential equivalent Minitest change, I think that should come under a separate PR

neanias commented 2 months ago

Left "you" in the CHANGELOG 🤦

neanias commented 2 months ago

God, really burning through the GHAction minutes with this one!

neanias commented 2 months ago

I can't merge this due to the failing compat check with Primer. Is that expected?

joelhawksley commented 2 months ago

The PVC failure is unrelated. Merging!