Skip to content
This repository has been archived by the owner on Jul 5, 2023. It is now read-only.

Rails application type generator #99

Conversation

bradleybuda
Copy link

@bradleybuda bradleybuda commented Jul 26, 2019

A first attempt to fix #98. The code is straightforward, but it has a (fatal) issue - the Rails#application type I'm generating in rake rbi:application conflicts with the Rails#application type defined in sorbet-typed, and I don't know what the resolution rules in sorbet are when two different .rbi files try to define different signatures on the same method - from playing around, I think the "last one in" wins, but it's not clear if this is well-defined or how we'd exploit it.

Some possible fixes:

  1. Remove Rails.application types from sorbet-typed/railties. This feels wrong because it makes sorbet less useful for Rails users who are not sorbet-rails users, and creates a backwards maintenance dependency from sorbet-typed to sorbet-rails.
  2. Update my generator to delete the railties type from the vendored sorbet/rbi/sorbet-typed/lib/railties/all/railties.rbi file. This is potentially fragile but it doesn't seem crazy to "manage" vendored types.
  3. Reach out to the sorbet team to find the "right way" for types to override other types (feels like a lot of complexity but maybe there are other use-cases for this?)

Thoughts? I'm going to proceed with option 2 for now.

@codecov-io
Copy link

codecov-io commented Jul 26, 2019

Codecov Report

Merging #99 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #99      +/-   ##
========================================
+ Coverage   98.95%    99%   +0.05%     
========================================
  Files         142    145       +3     
  Lines        1242   1306      +64     
========================================
+ Hits         1229   1293      +64     
  Misses         13     13
Impacted Files Coverage Δ
spec/support/v6.0/config/application.rb 100% <100%> (ø) ⬆️
spec/rake_rails_rbi_application_spec.rb 100% <100%> (ø)
lib/sorbet-rails/application_rbi_formatter.rb 100% <100%> (ø)
lib/sorbet-rails/tasks/rails_rbi.rake 94.44% <100%> (+0.69%) ⬆️
spec/support/v4.2/config/application.rb 100% <100%> (ø) ⬆️
spec/support/v5.1/config/application.rb 100% <100%> (ø) ⬆️
spec/support/v5.0/config/application.rb 100% <100%> (ø) ⬆️
spec/support/v5.2/config/application.rb 100% <100%> (ø) ⬆️
spec/support/v5.2-no-sorbet/config/application.rb 100% <100%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edc5f80...9738147. Read the comment docs.

Copy link
Contributor

@jasnow jasnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interested in this work.

@hdoan741
Copy link
Contributor

The conflict sig is a challenging issue. I think (2) is not the right approach because the file can contain other signatures as well. Removing the file will remove more than the application signature.
There is a (4) option: instruct users to remove the self.application sig from the railtie.rb file manually when using this rake task, but that's prone to error when people update the sorbet-typed library.

Let's post in sorbet-ruby slack and see what the sorbet team suggest.

@connorshea
Copy link
Contributor

Here's the Parlour version of the RBI generator:

root.create_class(Rails.application.class.name, superclass: 'Rails::Application')

rails_module = root.create_module('Rails')
rails_module.create_method(
  'application',
  return_type: Rails.application.class.name,
  class_method: true
)

@hdoan741
Copy link
Contributor

hdoan741 commented Dec 9, 2019

Closing because this PR has been outdated for a long time. @bradleybuda If you still want to make the change, please send another PR based on the newest code

@hdoan741 hdoan741 closed this Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type of Rails.application should be YourApp::Application
5 participants