Skip to content

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?

@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 reitermarkus force-pushed the master branch 2 times, most recently from 1bad124 to d09da1e Compare December 21, 2019 22:35
@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 :)

@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