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

Fix spelling errors in packet builder #79

Merged
merged 3 commits into from
Jan 5, 2024

Conversation

zheylmun
Copy link
Contributor

@zheylmun zheylmun commented Jan 2, 2024

Hello!
Not just a drive-by spell check, wanted to thank you for a great library, offer a bit of help, and ask if you'd be interested in having PTP support added. I've been working on it a bit separately. It's not nearly as common as most of the rest of the protocols, and it is a payload as opposed to a header, but figured I'd ask.

@zheylmun
Copy link
Contributor Author

zheylmun commented Jan 2, 2024

As a note, I goofed and was only seeing one file, so I thought the builder had just gotten missed. The actual spelling pass over the repo is quite large. Happy to add the rest of it if you'd like me to tack it on to this PR.
VoidstarSolutions@7fe9265

@JulianSchmid
Copy link
Owner

JulianSchmid commented Jan 4, 2024

Hi @zheylmun

As a note, I goofed and was only seeing one file, so I thought the builder had just gotten missed. The actual spelling pass over the repo is quite large. Happy to add the rest of it if you'd like me to tack it on to this PR. VoidstarSolutions@7fe9265

Thanks for the PR & flowers. Feel free to add it to this PR if you want to. I will wait with merging until I hear back from you.

Not just a drive-by spell check, wanted to thank you for a great library, offer a bit of help, and ask if you'd be interested in having PTP support added. I've been working on it a bit separately. It's not nearly as common as most of the rest of the protocols, and it is a payload as opposed to a header, but figured I'd ask.

Thanks for the offer, but I avoided including anything bellow the UDP & TCP layer until now. The waters kind of get muddy where I should stop adding stuff to etherparse. Also it is less easy to identify which protocol is contained within in a message as ports numbers are often configurable (there is no clear port-to-protocol mapping). So I kind of avoided that layer for these reasons 😅.

Whenever I needed something parsed on these layers I moved the parsing into separate crates (for example https://crates.io/crates/someip_parse & https://crates.io/crates/dlt_parse ). So if you want to do this, I would propose you move PTP parsing into a separate crate or start a new crate that specifically tackles parsing of protocols packaged in UDP or TCP.

Greets
Julian

@zheylmun
Copy link
Contributor Author

zheylmun commented Jan 5, 2024

Hello @JulianSchmid!
Pushed the rest of the changes. I've run the checks that I can locally but if you run CI I'll fix anything if I goofed on any of the other platforms.

Copy link
Owner

@JulianSchmid JulianSchmid left a comment

Choose a reason for hiding this comment

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

Thanks a lot ❤️.

The corrections I will commit on top myself (you don't need to do anything).

changelog.md Outdated Show resolved Hide resolved
etherparse/src/transport/icmpv4_slice.rs Outdated Show resolved Hide resolved
etherparse/src/transport/icmpv4_slice.rs Outdated Show resolved Hide resolved
etherparse/src/transport/icmpv4_slice.rs Outdated Show resolved Hide resolved
@JulianSchmid JulianSchmid merged commit 4c6c586 into JulianSchmid:master Jan 5, 2024
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.

2 participants