Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce new heuristics for block types #1180

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Introduce new heuristics for block types #1180

merged 2 commits into from
Dec 21, 2022

Conversation

soutaro
Copy link
Member

@soutaro soutaro commented Dec 19, 2022

Instead of making everything optional blocks, tries to infer if the block is optional or not based on the implementation.

  1. If block_given? is called in the method, the block is optional
  2. If the & var is tested in the method, the block is optional

Related to #1128

Not sure why it fails…
@soutaro
Copy link
Member Author

soutaro commented Dec 19, 2022

@ksss How about this one? Seems like the example you put in #1228 can be correctly handled.

@soutaro soutaro merged commit f84354c into master Dec 21, 2022
@soutaro soutaro deleted the prototype-block branch December 21, 2022 00:27
@ksss
Copy link
Collaborator

ksss commented Dec 21, 2022

@soutaro Thank you for great implementation.

But I'm sorry. My example was too few.

I don't think the problem of ruby/gem_rbs_collection#16 as a real case has been solved.

For example, ActiveSupport::Callbacks::ClassMethods#set_callback in https://github.com/rails/rails/blob/18b5f24e274bd4318749d8287214f6f2bca9bcad/activesupport/lib/active_support/callbacks.rb , it calls ActiveSupport::Callbacks::ClassMethods#normalize_callback_params internally and #normalize_callback_params is It works with or without block. Naturally, #set_callback also works with or without block.

Thus, the propagation of the requirement for blocks should be taken into account.

In this modification, ActiveSupport::Callbacks::ClassMethods#set_callback outputs the block as required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants