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

Make naming consistent #3

Merged
merged 4 commits into from
Dec 29, 2022
Merged

Make naming consistent #3

merged 4 commits into from
Dec 29, 2022

Conversation

vadorovsky
Copy link
Owner

  • Use short names like eth, hdr, proto, since that's what Rust stdlib is doing with network-related names.
  • Name the protocol enums after the frames they are included in. So, for example, EthProto for protocol info included in the Ethernet header.
  • Remove all from_be methods, callers of the library should do that.
  • Use proto enums directly as member field types, so we avoid manual conversion.

Signed-off-by: Michal Rostecki vadorovsky@gmail.com

@dmitris
Copy link
Contributor

dmitris commented Dec 23, 2022

$ cargo doc
Documenting network-types v0.0.2 (/Users/dmitris/dev/hack/gh/vadorovsky/network-types)
warning: this URL is not a hyperlink
--> src/l3/ip.rs:94:5
|
94 | /// https://en.wikipedia.org/wiki/List_of_IP_protocol_numbers
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: <https://en.wikipedia.org/wiki/List_of_IP_protocol_numbers>

src/l3/ip.rs Outdated Show resolved Hide resolved
@vadorovsky
Copy link
Owner Author

@dmitris Fixed, thanks for review!

Copy link

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Throwing in some general comments as well here since this is slated for inclusion in the book.
Naming wise, I think I actually prefer the verbosity unless the acronyms are well known - and I mean well known by people outside of networking... 😆

It might also be worth adding to the readme the philosohpy on naming for contributors so they can follow along when adding new protocols.

e.g

  1. Prefer field names as described in IEEE, IETF or other standards
  2. Where field names contain spaces, replace with _
  3. The following contractions are permitted: source -> src, destination -> dst, address -> addr etc...

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/l2/eth.rs Outdated Show resolved Hide resolved
src/l2/eth.rs Outdated Show resolved Hide resolved
src/l3/ip.rs Outdated Show resolved Hide resolved
src/l3/ip.rs Outdated Show resolved Hide resolved
src/l3/ip.rs Outdated Show resolved Hide resolved
src/l3/ip.rs Outdated Show resolved Hide resolved
src/l3/ip.rs Outdated Show resolved Hide resolved
@vadorovsky
Copy link
Owner Author

@dave-tucker

Naming wise, I think I actually prefer the verbosity unless the acronyms are well known

To be honest, I don't have any hard preference whether names should be short or verbose, but Alessandro's point for using the short ones (Ipv4Hdr, eth and so on) is that std::net is using shortened names and almost never fully verbose ones.

@dmitris
Copy link
Contributor

dmitris commented Dec 23, 2022

Re src/dst vs. src_addr/dst_addr, I think the most logical and easiest to remember would be the name that is closest to the ones used in the related Linux kernel structures. If have to choose between short and verbose, my preference would be on the short/succinct side.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/eth.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
@vadorovsky vadorovsky force-pushed the consistent-naming branch 2 times, most recently from 1db8272 to c70187d Compare December 27, 2022 10:56
* Use short names like `eth`, `hdr`, `proto`, since that's what Rust
  stdlib is doing with network-related names.
* Name the protocol enums after the frames they are included in. So, for
  example, `EthProto` for protocol info included in the Ethernet header.
* Remove all `from_be` methods, callers of the library should do that.
* Use proto enums directly as member field types, so we avoid
  manual conversion.

Signed-off-by: Michal Rostecki <vadorovsky@gmail.com>
Signed-off-by: Michal Rostecki <vadorovsky@gmail.com>
Signed-off-by: Michal Rostecki <vadorovsky@gmail.com>
@vadorovsky vadorovsky merged commit ab22fa6 into main Dec 29, 2022
@vadorovsky vadorovsky deleted the consistent-naming branch December 29, 2022 17:42
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.

None yet

4 participants