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

add serialization attributes for MacAddr enum #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alworx
Copy link

@alworx alworx commented Dec 16, 2020

I don't know if there is a particular reason for not having serde attributes on the MacAddr enum, but I needed this badly :)

@svartalf
Copy link
Owner

Hi, @alworx!

As far as I remember, serde derives were not added for MacAddr enum, because it was not clear what enum representation it should use — none of them will suite all possible cases.
Yet, considering that MacAddr6 and MacAddr8 structs already have these attributes, I see no issues with adding it for MacAddr too, even if it will be not really human-readable by default:

# In a pseudocode:

> serde_json::to_string(MacAddr6::nil())
[0,0,0,0,0,0]

> serde_json::to_string(MacAddr::from(MacAddr6::nil()))
{"V6":[0,0,0,0,0,0]}

I pushed 4c2305a into master branch in order to fix CI, can you rebase/merge your branch just to be sure that everything is okay?

@alworx
Copy link
Author

alworx commented Dec 16, 2020

I see, you're perfectly right.
I would have preferred untagged representation (i pushed it in 3c9a5ab), but string representation using colon or canonical format would be a better fit (for my use).
So don't feel pushed to merge the pull request :)

@svartalf
Copy link
Owner

svartalf commented Dec 22, 2020

I gave it a thought and it looks like current serde implementation was implemented badly and needs to be revisited.

What serde implements for IpAddr / IpAddrV4 / IpAddrV6 looks exactly like what we want: https://github.com/serde-rs/serde/blob/3c9fa1ccdf66b533113a2c8d8f7f6ff4fbb457b9/serde/src/ser/impls.rs#L653-L707

I like that this implementation support both human-readable and machine representations, it's also applicable to our case here.

  1. Human-readable representation can output "01-23-45-67-89-AB", which is canonical IEEE 802 format (same to MacAddr6)
  2. Machine-readable representation will just output the sequence of bytes same as now

The only issue I see is that if human-readable repr will use hyphen-separated format (which I'm inclined to have), it will clash with current std::fmt::Display implementation, which by default uses colon-separated output.
We might want to rethink this approach, because while it looks nice right now with formatting flags introduced in #2, it will be an inconsistent behavior.

For reference, that's how formatting flags are used right now:

assert_eq!(&format!("{}",    addr), "AB:0D:EF:12:34:56");
assert_eq!(&format!("{:-}",  addr), "AB-0D-EF-12-34-56");
assert_eq!(&format!("{:#}",  addr), "AB0.DEF.123.456");

As an additional note to myself, introducing any of those changes should be considered as breaking change and will require major version bump.

@1Dragoon
Copy link

1Dragoon commented Mar 11, 2023

What's wrong with this approach?

master...1Dragoon:rust-macaddr:master

Though IMHO we should be outputting in such a way that the delimiters are kept to a minimum, assuming we use them at all. My preference for JSON in particular (and the way I'm using it for my own project) is to just output the literal hex string with no delimiters. This makes it very space efficient when you have a very large number of MAC addresses in a given JSON output. To make it more readable, I also use lowercase hex.

Also, while it might make sense for IpAddr to have delimiters by default, I feel that MAC addresses should have none at all for the simple fact that there are no standard delimiters, which is different from IPv4 and IPv6. Thus, in a private fork, I made the default to_string() method output no delimiters with lowercase hex, while moving the colon delimiter to the plus sign. I also made the MacAddr6 output as 0102.0304.0506, also lowercase, which is a format common in networking equipment (I don't know where the splitting by three nibbles this crate uses comes from, I've never encountered that before.)

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

3 participants