Skip to content

Add RBS type definitions for lib/net/ping - #54

Open
eitoball wants to merge 1 commit into
masterfrom
add_rbs_sigunature
Open

eitoball wants to merge 1 commit into
masterfrom
add_rbs_sigunature

Conversation

@eitoball

@eitoball eitoball commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

What

Add RBS type signatures for the Net::Ping class hierarchy (Ping, TCP, UDP, ICMP, External, HTTP, WMI) under sig/net/ping/.

Why

The gem had no type definitions, so consumers using Steep/RBS-based type checking had no static type information for this library's public API.

Changes

  • Added sig/net/ping/{ping,tcp,udp,icmp,external,http,wmi,version}.rbs, mirroring lib/net/ping/.
  • Ping::WMI#ping's return type is declared as Struct::PingStatus (defined at the top-level Struct namespace, matching where Struct.new('PingStatus', ...) actually creates the constant at runtime), with a PingStatus: singleton(::Struct::PingStatus) constant alias inside WMI for the equivalent Net::Ping::WMI::PingStatus reference the source also creates.
  • Ping::TCP#ping and Ping::ICMP#ping are typed as Float | false | nil: the method body ends with @duration = ... if bool, which evaluates to the duration or nil on the normal path, but both methods also have explicit early return false branches on socket/timeout failures. UDP, External, and HTTP explicitly return bool and are typed accordingly.
  • Ping#exception is typed as (Exception | String | singleton(Errno::ECONNREFUSED))?: TCP/UDP/ICMP assign real exception instances (and TCP assigns the Errno::ECONNREFUSED class object itself on one branch), while External/HTTP only ever assign String messages.
  • Ping#port (and the corresponding constructor parameter, including the UDP/ICMP/HTTP initializer overrides) is typed as (Integer | String)?, since port= documents and examples/example_pingtcp.rb demonstrates passing a service-name string such as "http".
  • Ping::HTTP#redirect? (private) is typed as returning bool? since response && ... evaluates to nil when response is nil.
  • Ping::ICMP overrides the inherited port= as ((Integer | String)?) -> bot, since undef_method :port= in lib/net/ping/icmp.rb makes any call raise NoMethodError at runtime and RBS has no syntax to un-declare an inherited method; bot documents the call as unreachable, following the same convention stdlib sigs use for methods that always raise.
  • Ping::WMI#ping/#ping? accept Hash[Symbol | String, untyped] for options, since lib/net/ping/wmi.rb interpolates option keys without restricting them to symbols.
  • No changes to lib/ or net-ping.gemspec — the gemspec already globs **/*, so the new sig/ files are picked up automatically.

Validated with rbs validate and cross-checked the method surface against rbs prototype rb. Went through several rounds of Copilot review; all findings were verified against the implementation before being applied.

Copilot AI lite review requested due to automatic review settings September 20, 2026 12:12

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Unresolved moderate signature mismatches remain in the reviewed files.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 6 Medium severity

Open (6)
What changed in this PR

Adds RBS type definitions for the Net::Ping class hierarchy and public constants.

Changes:

  • Added signatures under sig/net/ping/.
  • Defined WMI status and version types.
  • Captured implementation-specific return types; no lib/ changes.
File Findings
sig/​net/​ping/​wmi.rbs Moderate (3 votes): Add the WMI::PingStatus constant alias.
sig/​net/​ping/​version.rbs No findings.
sig/​net/​ping/​udp.rbs No findings.
sig/​net/​ping/​tcp.rbs Moderate (3 votes): Include false in TCP#ping’s return type.
sig/​net/​ping/​ping.rbs Moderate (3 votes): Include String in exception’s type. Moderate (1 vote): Permit string ports in readers and constructors.
sig/​net/​ping/​icmp.rbs Moderate (3 votes): Include false in ICMP#ping; remove the inherited port= writer. Moderate (1 vote): Declare the optional initialization block.
sig/​net/​ping/​http.rbs Moderate (3 votes): Make private redirect?’s result nilable.
sig/​net/​ping/​external.rbs No findings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread sig/net/ping/http.rbs Outdated
Comment thread sig/net/ping/icmp.rbs
Comment thread sig/net/ping/icmp.rbs Outdated
Comment thread sig/net/ping/ping.rbs Outdated
Comment thread sig/net/ping/tcp.rbs Outdated
Comment thread sig/net/ping/wmi.rbs

Copilot AI 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.

Comment thread sig/net/ping/http.rbs Outdated
Comment thread sig/net/ping/icmp.rbs
Comment thread sig/net/ping/icmp.rbs Outdated
Comment thread sig/net/ping/icmp.rbs Outdated
Comment thread sig/net/ping/ping.rbs Outdated
Comment thread sig/net/ping/tcp.rbs Outdated
Comment thread sig/net/ping/wmi.rbs Outdated

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Address the three moderate type-definition issues concerning service-name ports and exception class objects.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Resolved since last review (7)

Comment thread sig/net/ping/ping.rbs Outdated
Comment thread sig/net/ping/ping.rbs Outdated

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Two moderate signature issues and related nits remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Resolved since last review (2)

Comment thread sig/net/ping/ping.rbs

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Unresolved findings cover CI validation, initializer self typing, WMI option keys, and inaccurate return-type documentation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 2 Low severity

Open (3)

Comment thread sig/net/ping/wmi.rbs Outdated
Comment thread sig/net/ping/tcp.rbs
@eitoball
eitoball force-pushed the add_rbs_sigunature branch 2 times, most recently from c98e0c3 to 9a5f643 Compare September 20, 2026 13:57
@eitoball
eitoball requested a lite review from Copilot September 20, 2026 14:06

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Three moderate constructor block typing issues remain in the External, TCP, and WMI signatures.

Review effort: Lite
Findings: None

Resolved since last review (3)

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Ping#initialize needs a self-typed block so subclass receivers are typed correctly.

Review effort: Lite
Findings: None

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Critical and moderate RBS signature issues remain in HTTP, ICMP, and UDP definitions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)

Comment thread sig/net/ping/http.rbs Outdated

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Add the missing ICMP#initialize block signature before approval.

Review effort: Lite
Findings: None

Resolved since last review (1)

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The HTTP redirect signature does not allow a nil port that the implementation can pass to do_ping.

Review effort: Lite
Findings: None

Covers Ping and its TCP/UDP/ICMP/External/HTTP/WMI subclasses. Validated with `rbs validate`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

A moderate unresolved port-type mismatch remains in sig/net/ping/http.rbs.

Review effort: Lite
Findings: None

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Three moderate issues and one documentation nit remain unresolved.

Review effort: Lite
Findings: None

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.

2 participants