pocke / rbs_rails

Apache License 2.0
283 stars 33 forks source link

Add RbsRails::ActionController::Generator #255

Closed tk0miya closed 1 year ago

tk0miya commented 1 year ago

To support Action Controllers strongly depend on Rails autoloader, add a new generator RbsRails::ActionController::Generator. It generates module definition automatically if controller classes are defined under modules.

This helps to generate correct types for Rails app defining controllers using shorthands (ex. class Foo::Bar::BazController).

pocke commented 1 year ago

ActionController does not provide this feature. It is Zeitwerk's feature, and it's available on other directories, such as models.

I understand this problem. This feature introduced many "missing class/module as namespace". But I do not want to add this feature to RBS Rails.

Because other layers can solve this problem. I have two ideas to fix this problem.

tk0miya commented 1 year ago

ActionController does not provide this feature. It is Zeitwerk's feature, and it's available on other directories, such as models.

I understand that. If this package is "rbs_actioncontroller", I'll withdraw this PR. But this is "rbs_rails". It's better to support generating types including Zeitwerk behaviors, isn't it?

Additionally, RbsRails::ActiveRecord::Generator has supported generating missing modules. I think this is not a new feature for rbs_rails. https://github.com/pocke/rbs_rails/blob/master/lib/rbs_rails/active_record.rb#L94-L110

Do you think it's better to remove this code from AR::Generator?

Note: I must admit generating empty modules and classes are ugly. But that helps to introduce Ruby types to newbies (including me), I believe.

pocke commented 1 year ago

It's better to support generating types including Zeitwerk behaviors, isn't it?

Right. Zeitwerk is part of Rails, so RBS Rails should generate types for Zeitwerk if necessary.

But RBS Rails does not need to implement this feature. Because rbs prototype runtime can do the same thing. If RBS Rails implement this feature, it's a Rails-specific feature. But if rbs prototype runtime implement it, we can widely use the feature.

For example, rbs prototype runtime can provide the following feature.

# Generate class definitions for all classes and modules.
$ rbs prototype runtime --require-relative config/environment.rb --only-classes

# Same as the above, but only for classes defined under `app/` directory.
# This filtering will be needed to avoid generating unexpected RBS.
$ rbs prototype runtime --require-relative config/environment.rb --only-classes --const-defined-under app/

The feature of rbs prototype prototype covers the necessity of your proposal. And we can use the feature for non-Rails projects.

If there is some reason to specialize in Rails, I think implementing it in RBS Rails is acceptable. Please tell me the reason if you have 🙏


Additionally, RbsRails::ActiveRecord::Generator has supported generating missing modules. I think this is not a new feature for rbs_rails.

Right. This feature is necessary to make the generated RBS work standalone. RBS Rails is responsible for the generated RBSs work without other generators.

So I do not have any plan to remove this feature for now. We can remove this feature in the future if rbs provides a feature to avoid missing classes error. For example:

# It does the same thing as the AR::Generator's feature
$ rbs fill-missing-classes generated.rbs

# Or rbs can ignore the missing classes
$ rbs validate --ignore-missing-classes
tk0miya commented 1 year ago

Thank you for your explanation. I'm withdrawing this now.