search

nshki / chusaku

Annotate your Rails controllers with route info.
https://rubygems.org/gems/chusaku
MIT License
87 stars 4 forks source link

Also look for controllers outside /app #37

Closed pineman closed 10 months ago

pineman commented 10 months ago

Hi! Thank you for this gem!

We're using packs-rails in our rails app, which means we've got controllers outside the /app directory. If you think it makes sense, could you consider supporting this use-case? I found this way to be the most minimal change required to support it.

Thank you!

nshki commented 10 months ago

Hi there! Thanks for the PR! I think your use case is very valid and I appreciate you trying to go about it in the least amount of change possible.

I'm wondering if there could be some conflicts with **/controllers/**/*_controller.rb being the default controllers pattern. I could see some codebases matching something like this in some kind of test directories, for example.

What do you think about adding a new flag to the CLI class so that folks can pass in a custom controllers pattern? e.g.

bundle exec chusaku --controllers-pattern='**/controllers/**/*_controller.rb'
bundle exec chusaku -c '**/controllers/**/*_controller.rb'
pineman commented 10 months ago

@nshki Yeah that's much better 😄 I thought about it but I didn't have time.

I've added the cli flag - WDYT?

pineman commented 10 months ago

Of course 🤦 sorry it was 3AM 😄

I've added the test you mentioned, plus an "integration" test for Chusaku.call. Just a negative test that calling chusaku with an invalid controllers_pattern doesn't do anything, as I thought adding a positive test with a different mock folder structure wasn't worth it. I had to mess with the Rails.root mock though. Changed it to return a "real" Pathname object just like Rails does, so that .join(controllers_pattern) works with whatever value. Let me know what you think.

pineman commented 10 months ago

Man, am I bad at linting or what? sorry.

Also, I noticed this: https://github.com/nshki/chusaku/blob/918ed5769d4e9007ef8ef08da5bbca674a711dd9/test/chusaku_test.rb#L68 assert docs says it only tests that the first argument is truthy. I wonder if this should be an assert_equal? Asserting the number of files written is two.

nshki commented 10 months ago

No need to apologize—I appreciate you a lot for taking the time to write this PR!

RE: the assert. Great call out. That definitely should be an assert_equal. I can clean that up in a separate PR. Or if you'd like to, you're more than welcome to issue a second PR for that.