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

Update Parlour and resolve root namespaces #170

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dorner
Copy link
Contributor

@dorner dorner commented Jul 19, 2023

This PR updates the version of Parlour to the latest (8.x) and also fixes a bug where the root Namespace Parlour type was not being parsed. 

tapioca exports classes in fully namespaced forms, so e.g. it prints this for the ruby-kafka gem:

class Kafka::Client
  def alter_configs(broker_id, configs = nil); end
  def alter_topic(name, configs = nil); end
  ...
end

This wasn't being picked up by sord because the Kafka constant is actually a Parlour::RbiGenerator::Namespace, which was not in the list of checked classes.

This PR allows for continuing to check the children of the root Namespace class without adding the namespace itself to the list of paths.

Note - took another look at this and Parlour is no longer shipping with an rbi folder - in fact, it seems like having an rbi folder in gems is no longer the accepted practice. This caused tests to fail. I've combined my previous PR with this and changed it so that it no longer checks these directories and in fact only checks the sorbet local directory.

Note 2 - looked again and other than issue #101 I haven't actually seen other problems with this. Going to put the RBI back and I found a different way to make the test pass.

@dorner
Copy link
Contributor Author

dorner commented Jul 24, 2023

I'm rethinking this as per my comments in #172 . Would love to hear feedback on that before merging this.

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.

1 participant