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

Merge sigs in RBI files with existing documentation #170

Merged
merged 7 commits into from
Jan 10, 2023
Merged

Conversation

dduugg
Copy link
Owner

@dduugg dduugg commented Dec 29, 2022

This PR updates the sig handler to check the YARD::Registry for existing documentation. If found, it will incorporate the sig information into the existing documentation. Otherwise, it will proceed as before, converting the sig into YARD documentation for normal processing.

wdyt @KaanOzkan @jscheid

Resolves #141

@dduugg dduugg marked this pull request as draft December 29, 2022 19:48
@dduugg dduugg force-pushed the merge-rbi-sigs branch 2 times, most recently from a7d3a18 to a31f403 Compare December 30, 2022 05:50
@dduugg dduugg marked this pull request as ready for review December 30, 2022 05:51
@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (2375f3f) compared to base (601ba16).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #170   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          753       826   +73     
=========================================
+ Hits           753       826   +73     
Impacted Files Coverage Δ
lib/yard-sorbet/handlers/sig_handler.rb 100.00% <100.00%> (ø)
lib/yard-sorbet/node_utils.rb 100.00% <100.00%> (ø)
spec/yard_sorbet/handlers/sig_handler_spec.rb 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dduugg
Copy link
Owner Author

dduugg commented Dec 30, 2022

I had some time to add (limited) attr support, so marking this as ready for review. Note that it seems that the primary interest here is in tapioca output, and AFAICT tapioca doesn't output attr*s.

The limited support is that we will only attempt to merge documentation if an attr* has a single parameter. The reason is that we remove the docstring once we've merged it into an existing docstring in the registry. If we support multiple attr* parameters, we may see a mix of registered and unregistered attributes. That would add a lot of complexity to the AST rewrite, for what feels like a niche use of rbi style.

@dduugg dduugg changed the title [WIP] Merge sigs in RBI files with existing documentation Merge sigs in RBI files with existing documentation Dec 30, 2022
@dduugg dduugg force-pushed the merge-rbi-sigs branch 4 times, most recently from a981c4e to a752d7c Compare January 1, 2023 17:07
@dduugg dduugg force-pushed the main branch 4 times, most recently from ef9a109 to e297077 Compare January 2, 2023 07:09
@dduugg dduugg force-pushed the merge-rbi-sigs branch 3 times, most recently from c344657 to cddc4b8 Compare January 3, 2023 00:22
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

LGTM as far as documenting RBIs more accurately 🙂

@dduugg dduugg merged commit c038be1 into main Jan 10, 2023
@dduugg dduugg deleted the merge-rbi-sigs branch January 10, 2023 01:53
@jscheid
Copy link

jscheid commented Mar 3, 2023

Hi guys, thanks for building this! Sorry I didn't get a chance to take a look at it before. I'm trying to implement it now but I'm running into SystemStackError: stack level too deep. This is with yard server.

I've already doubled the stack, then 10x, by setting RUBY_THREAD_VM_STACK_SIZE in the shell that invokes bundle exec yard server (thinking that maybe it's not infinite recursion, just hungry) but same result. I'm not yet sure how I can verify that the setting took hold, any ideas?

Here is the full backtrace with RUBY_THREAD_VM_STACK_SIZE=20971520: https://gist.github.com/jscheid/73177e982b2983e4aeb94e590043429e

@dduugg
Copy link
Owner Author

dduugg commented Mar 3, 2023

@jscheid to be clear, is bumping the yard-sorbet version the only change that you've made to produce the error? A few things to try:

  1. There's a yard PR to fix a stack overflow issue, does that resolve your problem?
  2. Can you bisect your codebase to find the offending file(s)? (Trim the contents consumed by yard until you have the smallest subset that will repro the problem.)
  3. Try adding some print lines to display the node source, and see if there's something that might be triggering a stack loop.

Let me know if you uncover any hints this way.

@jscheid
Copy link

jscheid commented Mar 3, 2023

@jscheid to be clear, is bumping the yard-sorbet version the only change that you've made to produce the error?

There's no stack overflow if I don't specify the RBI files on the command line.

Let me know if you uncover any hints this way.

OK, I'll dig in. I just thought you had any ideas just from looking at the backtrace.

@Skipants
Copy link

Skipants commented Mar 15, 2023

@jscheid Definitely try yard-sorbet with the fix to the yard gem @dduugg linked above.

If yard-sorbet adds more objects to YARD::Registry, which it seems to do, then that PR could fix your issue. The problem solved by that PR is that YARD currently does a recursive call that can blow up the call stack at a pace exponential to the size of that YARD::Registry. That causes the stack overflow.

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.

Merge tapioca dsl output
4 participants