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

Support core::fmt::Display and alternative formats. #2

Merged
merged 8 commits into from
Dec 29, 2019

Conversation

reitermarkus
Copy link
Contributor

No description provided.

@svartalf
Copy link
Owner

Hey, @reitermarkus! Thanks for a pull request :)

Can we start with the master rebasing? Apparently, I broke the CI in here and it is fixed now in 16784c4

@svartalf
Copy link
Owner

I checked the docs about the MAC addresses and have a proposal to change the formatting a bit.

There are three possible ways to display the address:

  1. hypen notation (00-00-00-00-00-00)
  2. colon notation (00:00:00:00:00:00)
  3. period-separated notation (000.000.000.000)

We can make the colon notation to be the default formatting option, use Formatter::sign_minus to display the hypen notation and Formatter::alternate to display the period-separated notation; that way we could cover all three notations at once.

I'm not sure about your default formatting 000000000000, though, is it used anywhere in a wild?

src/addr6.rs Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Contributor Author

I'm not sure about your default formatting 000000000000, though, is it used anywhere in a wild?

I haven't seen it. I just thought having no separators by default and having to choose colons/hyphens explicitly would make sense.

@reitermarkus
Copy link
Contributor Author

I left the other formats as is and added {:.0} for the period-separated notation.

@svartalf
Copy link
Owner

Dot format is used for a numeric types and afaik can't be used without supplying the precision, so it is impossible to use it as a {:.}, arbitrary precision should be supplied always, as in {:.0}.

This does not seems to be very convinient and easy to remember; had you thought on my idea about formatting options? I would love to hear your thoughts on that.

@reitermarkus
Copy link
Contributor Author

I changed it now to your suggestion.

Copy link
Owner

@svartalf svartalf left a comment

Choose a reason for hiding this comment

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

That looks great! There is a small thing I would like to clarify, after that I'll be glad to merge and publish this PR :)

src/addr6.rs Outdated Show resolved Hide resolved
@svartalf svartalf merged commit 0f0916a into svartalf:master Dec 29, 2019
@svartalf
Copy link
Owner

Awesome, everything looks great! I'm going to change some things in CI, check the results and will try to publish your change today

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