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

HexFormat proposal #361

Merged
merged 1 commit into from
Oct 11, 2023
Merged

HexFormat proposal #361

merged 1 commit into from
Oct 11, 2023

Conversation

qurbonzoda
Copy link
Contributor

No description provided.


## Motivation

Our research has shown that hexadecimal representation is more widely used than other numeric bases,
Copy link

Choose a reason for hiding this comment

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

Citation needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any public document with the results. But search in grep.app finds much more usages of toString(16) than any other base.

Copy link

Choose a reason for hiding this comment

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

Probably .toString( /* 10 */ ) is more common, should we also consider introducing DecFormat?
I'm not against this proposal; it seems strange to tackle hexadecimal number formatting before decimal ones.

Copy link
Contributor Author

@qurbonzoda qurbonzoda Sep 12, 2023

Choose a reason for hiding this comment

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

The decimal representation of numeric values is indeed more common, and this document explicitly agrees with the proposition. However, this proposal is about introducing an API to facilitate formatting (and parsing) the hex representation of binary data. It provides use cases and proposes an API to make it easy to handle those use cases.
Of course, there are other potentially more impactful features. They deserve separate proposals.
While HexFormat is not the most important or useful feature out there, it does address the pain points outlined in this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Discussion of the proposal has been moved here: #362
Please don't hesitate to express your concerns there.

macAddress.toHexString(threeGroupFormat) // "001B.6384.45E6"
```

## Alternatives
Copy link

Choose a reason for hiding this comment

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

Another option is a third-party implementation. Logging, for example, is only available this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you also please provide a link to the library you are referring to?

Copy link

Choose a reason for hiding this comment

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

```

`BytesHexFormat` and `NumberHexFormat` classes hold format options for `ByteArray` and numeric values, correspondingly.
`uppercase` option, which common to both `ByteArray` and numeric values, is stored in `HexFormat`.
Copy link

Choose a reason for hiding this comment

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

Hi,
I think the uppercase option is vague.
Does uppercase = false means force all letters to lowercase or does it mean keep the letters as is (not touch them)?
Can it be renamed to something like forceUppercase or similar?

```

`BytesHexFormat` and `NumberHexFormat` classes hold format options for `ByteArray` and numeric values, correspondingly.
`uppercase` option, which is common to both `ByteArray` and numeric values, is stored in `HexFormat`.
Copy link
Member

Choose a reason for hiding this comment

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

We should likely discuss the name of this option once again. While it is named upperCase in code, in other places of KEEP, it is referred to as uppercase, which may indicate that it could be a more natural choice of the name, also more consistent with the name of the String/Char.uppercase() function.

Another approach could use some prefix, for example, isUpperCase or useUpperCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I added a comment regarding the option name in the proposal discussion issue: #362 (comment)
Hopefully, some naming variants will be proposed by the community.

@qurbonzoda qurbonzoda merged commit 8989207 into master Oct 11, 2023
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.

4 participants