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

[resolv] Add RBS file for Resolv #697

Merged
merged 14 commits into from
Jul 12, 2021
Merged

Conversation

HoneyryderChuck
Copy link
Contributor

Added RBS signatures for the resolv package, + some tests.

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Please fix the name resolution errors like dns_name, Sender, etc... Write the type names in qualified style or declare classes/modules in nested style.

I also found socket classes are undefined. I think writing socket RBS is too much for this, so just adding empty classes in a file like resolv/0/sockets.rbs is good.

@HoneyryderChuck
Copy link
Contributor Author

I also found socket classes are undefined.

@soutaro I noticed it while reviewing the errors, and went ahead with it here. Let's deal with it first, then I'll revisit this.

@HoneyryderChuck
Copy link
Contributor Author

@soutaro it's ready for review.

Write the type names in qualified style or declare classes/modules in nested style.

I didn't understand this comment. Can you elaborate?

@soutaro
Copy link
Member

soutaro commented Jul 11, 2021

@HoneyryderChuck Could you remove the trailing whit spaces?

@HoneyryderChuck
Copy link
Contributor Author

@soutaro done.

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

👏

@soutaro soutaro merged commit 9efc539 into ruby:master Jul 12, 2021
@HoneyryderChuck HoneyryderChuck deleted the resolv branch July 12, 2021 07:01
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