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

Fix output of binary, Identity, Address for SQL output and the 'Display' of them to show a full hex value #1087

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Apr 12, 2024

This is a companion for Implement 'SQL' parsing for 'Identity, Address' in hex format, so now we have a correct round-trip of binary values in SQL.

Now when printing an SQL output that has Binary, Identity, or Address fields it correctly displays them as a hex value.

🪐quickstart>select i, i from OneIdentity
 i                                                                  | i
--------------------------------------------------------------------+--------------------------------------------------------------------
 0x93dda09db9a56d8fa6c024d843e805d8262191db3b4ba84c5efcd1ad451fed4e | 0x93dda09db9a56d8fa6c024d843e805d8262191db3b4ba84c5efcd1ad451fed4e
 0x93dda09db9a56d8fa6c024d843e805d8262191db3b4ba84c5efcd1ad451fed2e | 0x93dda09db9a56d8fa6c024d843e805d8262191db3b4ba84c5efcd1ad451fed2e

Description of Changes

Please describe your change, mention any related tickets, and so on here.

API and ABI breaking changes

I also change the Display of Identity & Address that mar this as potentially breaking.

Before they print like 00000000000000000000000000000000 but now 0x00000000000000000000000000000000.

If desired then I could revert that part. Because the print of `SQL' is changed for these types is also a breaking change anyway (if only for the small chance somebody depend in the old way of display them).

Expected complexity level and risk

2

Is there any implication in change the Display of Identity & Address?

Testing

  • output_identity_address confirmed it print correctly for SQL
  • Manual inspection of SQL output using the cli.

@mamcx mamcx added release-any To be landed in any release window bitcraft issue Active issue for the BitCraft team api-break A PR that breaks some user-visible API labels Apr 12, 2024
@mamcx mamcx requested a review from coolreader18 April 12, 2024 14:27
@mamcx mamcx self-assigned this Apr 12, 2024
@@ -38,7 +38,7 @@ impl Default for Address {

impl fmt::Display for Address {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.pad(&self.to_hex())
write!(f, "0x{}", self.to_hex())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes this a breaking change. Should I remove this?

@@ -99,7 +99,7 @@ impl Identity {

impl fmt::Display for Identity {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.pad(&self.to_hex())
write!(f, "0x{}", self.to_hex())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes this a breaking change. Should I remove this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that breaks some user-visible API bitcraft issue Active issue for the BitCraft team release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants