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

Parse methods without sigs #76

Merged
merged 4 commits into from
Apr 12, 2020

Conversation

jeffcarbs
Copy link
Contributor

@jeffcarbs jeffcarbs commented Apr 12, 2020

Fixes #69

I created a new method based on parse_sig_into_methods called parse_method_into_methods. This new method will generate RbiGenerator::Methods and RbiGenerator::Attributes from unsigged methods, setting any parameter types and return types to T.untyped.

I had started by trying to adapt the parse_sig_into_methods to handle the sig not being present but there were a lot of validations sprinkled throughout the method and adding a bunch of conditionals made it much harder to read. One idea on how to remove some of the duplication:

  • Leave the parse_method_into_methods as-is, basically take a method call or attr_* and return a method/attribute that is totally untyped.
  • Have the parse_sig_into_methods
    • Parse the sig
    • Call the parse_method_into_methods to generate untyped methods/attributes
    • Loop over the returned methods/attributes and apply the relevant validations and add types where possible.

I'd lean toward leaving this PR as-is (i.e. with the duplication) and applying this or some other type of refactor as a follow up if you wanted to. There would probably be some personal preference involved in how you actually DRYed up the code so might make more sense as something you'd do vs a contributor.

@AaronC81
Copy link
Owner

This is absolutely superb. Thank you ever so much!

I'll probably add a flag to revert to the old behaviour just in case people want that, but use this as the new default.

I'm thinking about refactoring TypeParser at some point soon anyway (#72), so I'll look into DRYing this up then.

Thanks again!

@AaronC81 AaronC81 merged commit bb8ad4a into AaronC81:master Apr 12, 2020
@jeffcarbs jeffcarbs deleted the parse-methods-without-sigs branch May 15, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for defs without sigs
2 participants