Skip to content

Add RBS signatures from ruby/rbs repo - #279

Merged
hsbt merged 3 commits into
ruby:masterfrom
sferik:add-rbs-signatures
Mar 17, 2026
Merged

Add RBS signatures from ruby/rbs repo#279
hsbt merged 3 commits into
ruby:masterfrom
sferik:add-rbs-signatures

Conversation

@sferik

@sferiksferik commented Mar 6, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

See #278 for context.

@sferiksferik mentioned this pull request Mar 6, 2026
Closed
ksss
ksss reviewed Mar 9, 2026
View reviewed changes

@ksssksss left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The test code differs significantly from the ruby/rbs version, making it difficult to pinpoint the differences.
Can it be made as close to the original as possible?
If there are issues with the original, please let me know.

Comment thread.github/workflows/sig.yml Outdated
Comment threadRakefile
@sferik
sferikforce-pushed the add-rbs-signatures branch 2 times, most recently from 1c7e9f0 to 0533460CompareMarch 10, 2026 13:41
@sferik

sferik commented Mar 10, 2026

Copy link
Copy Markdown
ContributorAuthor

Okay, I've copied the tests from ruby/rbs verbatim and added the rbs:annotate rake tasks to this PR.

ksss
ksss reviewed Mar 11, 2026
View reviewed changes

@ksssksss left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NetHTTPTestSigHelper and NetHTTPRBSTestCase should be removed.
This approach does not allow us to confirm if the manifest.yaml specification is sufficient.

Additionally, library should be used.
This is a DSL that explicitly states that only the specified library has been loaded.

library"net-http"testing"singleton(::Net::HTTP)"

@sferik

sferik commented Mar 11, 2026

Copy link
Copy Markdown
ContributorAuthor

@ksss How does that look?

ksss
ksss reviewed Mar 11, 2026
View reviewed changes
Comment threadsig/manifest.yaml Outdated
Comment threadGemfile
@sferik
sferikforce-pushed the add-rbs-signatures branch from c8545ed to e3b1e4bCompareMarch 11, 2026 19:10
ksss
ksss reviewed Mar 12, 2026
View reviewed changes

@ksssksss left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it's almost good.
The test_sig should be excluded from spec.files in net-http.gemspec at the end..

@sferik
sferikforce-pushed the add-rbs-signatures branch from e3b1e4b to 26c3060CompareMarch 12, 2026 01:15
@sferik

sferik commented Mar 12, 2026

Copy link
Copy Markdown
ContributorAuthor

@ksss Done.

@sferik
sferikforce-pushed the add-rbs-signatures branch from 26c3060 to 23a1043CompareMarch 12, 2026 01:17
@sferik
Add existing RBS interface tests from ruby/rbs repo
2842ca5
@sferik
Use library DSL in RBS tests
5191d32
Replace custom NetHTTPTestSigHelper/NetHTTPRBSTestCase with the standard
RBS::UnitTest library DSL, which validates that manifest.yaml declares
all required dependencies.
@sferik
sferikforce-pushed the add-rbs-signatures branch from 23a1043 to 5191d32CompareMarch 12, 2026 01:18
ksss
ksss reviewed Mar 12, 2026
View reviewed changes

@ksssksss left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@sferik

sferik commented Mar 12, 2026

Copy link
Copy Markdown
ContributorAuthor

@ksss Thank you for taking the time to thoroughly review it.

@hsbt

hsbt commented Mar 17, 2026

Copy link
Copy Markdown
Member

@sferik@nurse approved this proposal at dev-meeting. I will merge this.

@hsbt
hsbt merged commit 59c4412 into ruby:masterMar 17, 2026
26 checks passed
@sferik

sferik commented Mar 17, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Thank you very much! Did you discuss #280 and #281 at your dev meeting? I just rebased those from the master branch.

@hsbt

hsbt commented Apr 1, 2026

Copy link
Copy Markdown
Member

@sferik Sorry my late response.

We briefly discussed #280 and #281 at the dev meeting. The direction looks fine, but since the main code has changed, we want to make sure there are no regressions before merging. That's as far as the discussion went.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@ksssksssksss left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sferik@hsbt@ksss