Skip to content

Fix Net::Ping::VERSION - #37

Merged
eitoball merged 1 commit into
eitoball:masterfrom
taketo1113:fix-gem-version
Aug 8, 2026
Merged

Fix Net::Ping::VERSION#37
eitoball merged 1 commit into
eitoball:masterfrom
taketo1113:fix-gem-version

Conversation

@taketo1113

Copy link
Copy Markdown

This Pull Request fixes Net::Ping::VERSION.

The version in the net-ping gem's gemspec is 2.0.8, but Net::Ping::VERSION is set to 1.7.6.
To resolve this inconsistency, Net::Ping::VERSION has been moved to a separate file, and the gemspec now references Net::Ping::VERSION from that file.

Steps to reproduce

  • ruby: 3.4.1
  • net-ping: 2.0.8
require 'net/ping'
Net::Ping::VERSION

Expected behavior

Net::Ping::VERSION
=> 2.0.8

Actual behavior

Net::Ping::VERSION
=> 1.7.6

@eitoball eitoball left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sorry for the delayed review.

[P1] Load Net::Ping::VERSION at runtimelib/net/ping/version.rb:4
Although it is loaded from net-ping.gemspec, version.rb is not required through the usual require 'net/ping' or require 'net/ping/tcp' paths. As a result, Net::Ping::VERSION raises a NameError, and the version test updated in test/test_net_ping.rb also fails. To preserve the public API, require ping/version from the base class or each public entry point.

@taketo1113
taketo1113 force-pushed the fix-gem-version branch 2 times, most recently from b7ce612 to d68267f Compare July 28, 2026 13:21
@taketo1113

Copy link
Copy Markdown
Author

@eitoball I rebased the branch and added require 'ping/version' to lib/net/ping.rb.

@eitoball eitoball left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the change! I left two comments — could you take a look?"

Comment thread lib/net/ping.rb Outdated
#
require 'rbconfig'

require_relative 'ping/version'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The "README.md" explicitly documents require 'net/ping/tcp'-style individual requires as a way to reduce memory footprint. Under that usage, "version.rb" is never loaded, so Net::Ping gets defined without the constant, and accessing Net::Ping::VERSION raises NameError.

So I suggest to change "lib/net/ping/ping.rb" like:

require_relative 'version'

class Ping
  VERSION = Net::Ping::VERSION

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I moved it from lib/net/ping.rb to lib/net/ping/ping.rb.

Comment thread test/test_net_ping.rb Outdated

@eitoball eitoball left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks! Looks good.
Please rebase and resolve a conflict, so that I can merge this PR.

@taketo1113

Copy link
Copy Markdown
Author

Thank you for reviewing.
I have rebased it!

@eitoball eitoball left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM — correct fix for the VERSION drift bug, follows standard Ruby gem versioning conventions, no packaging or behavior concerns.

@eitoball
eitoball merged commit 940faac into eitoball:master Aug 8, 2026
14 checks passed
@taketo1113
taketo1113 deleted the fix-gem-version branch August 8, 2026 12:30
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