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

Print returned TTL value #345

Merged
merged 1 commit into from
Aug 31, 2024
Merged

Conversation

nalves599
Copy link
Contributor

Print the returned TTL value from the IP header.

@auerswal
Copy link
Collaborator

Thanks for this pull request! I have started looking at the changes and will provide a first review soon.

Copy link
Collaborator

@auerswal auerswal left a comment

Choose a reason for hiding this comment

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

This looks patterned after the --print-tos option, I like that. I also like that you added tests and documentation.

The code itself looks OK, but please address my comments regarding tests and man page.

ci/test-04-options-a-b.pl Outdated Show resolved Hide resolved
ci/test-11-unpriv.pl Outdated Show resolved Hide resolved
doc/fping.pod Outdated Show resolved Hide resolved
ci/test-04-options-a-b.pl Outdated Show resolved Hide resolved
Copy link
Collaborator

@auerswal auerswal left a comment

Choose a reason for hiding this comment

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

I hope that the tests now work as intended.
Please adjust the man page description (I mentioned hop count only for background, not as something that be added to the man page).

doc/fping.pod Outdated Show resolved Hide resolved
Co-authored-by: Luís Fonseca <[email protected]>
@coveralls
Copy link

coveralls commented Aug 24, 2024

Coverage Status

coverage: 87.622% (-0.1%) from 87.746%
when pulling ddf417e on nalves599:show-ttl-flag
into c0eb514 on schweikert:develop.

@nalves599 nalves599 requested a review from auerswal August 24, 2024 17:49
Copy link
Collaborator

@auerswal auerswal left a comment

Choose a reason for hiding this comment

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

I'd say this looks mostly OK.

The commit message could be a bit more explicit that this introduces a new option, but then your example commit has a similarly brief commit message.

I'll wait a bit before merging. This allows me to think more about the pull request, and gives others an opportunity to look at it, too.

@auerswal auerswal merged commit 291656a into schweikert:develop Aug 31, 2024
7 of 9 checks passed
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.

3 participants