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 HexString method for namespace id #201

Merged
merged 3 commits into from
May 26, 2023

Conversation

walldiss
Copy link
Member

Overview

Implements #200

@walldiss walldiss requested a review from rootulp May 25, 2023 14:07
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #201 (54ce402) into master (ba0ebb8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #201   +/-   ##
=======================================
  Coverage   95.39%   95.40%           
=======================================
  Files           5        5           
  Lines         565      566    +1     
=======================================
+ Hits          539      540    +1     
  Misses         15       15           
  Partials       11       11           
Impacted Files Coverage Δ
namespace/id.go 100.00% <100.00%> (ø)

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

oooh nice!

this might actually be more useful in https://github.com/celestiaorg/celestia-app/blob/578c71cbfe033c1b597b903d22fda06f772db816/pkg/namespace/namespace.go#L8

since I think this namespace doesn't include the namespace verison, correct @rootulp? which could lead to devs using the incorrect hex

@rootulp
Copy link
Collaborator

rootulp commented May 25, 2023

This namespace does include the namespace version so I think we can include this change here.

@rootulp rootulp added the enhancement New feature or request label May 25, 2023
@Wondertan
Copy link
Member

Wondertan commented May 25, 2023

Any reason not to change the default behavior of String to be always hex encoded? The current String is sorta useless while having default human-readable support for Stringer everywhere is not, AFAICS. The current String implementation can still be done with string(ID) if anyone needs raw nid in string format.

@evan-forbes, @rootulp, thoughts?

@rootulp
Copy link
Collaborator

rootulp commented May 25, 2023

My rationale for recommending a new HexString() method is to avoid breaking compatability

changing existing String() method (would break compatibility)

If we can verify that no consumers of this library rely on the existing string representation then I think we can make the breaking change and modify the existing String() method

Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

LGTM!

@staheri14
Copy link
Contributor

@walldiss Could you please revise the title of the PR to capitalize HexString? otherwise, it might be misleading, suggesting that the introduced method is private.

@walldiss walldiss changed the title Add hexString method for namespace id Add HexString method for namespace id May 26, 2023
@Wondertan
Copy link
Member

If we can verify that no consumers of this library rely on the existing string representation then I think we can make the breaking change and modify the existing String() method

I verified the usage in the node and it's used only in places where human-readable string is expected.

@evan-forbes
Copy link
Member

evan-forbes commented May 26, 2023

I'm fine with mergin either, but I tend to agree that returning hex makes a lot of sense. if we need a string() for some reason, we can just call string(id)

@rootulp
Copy link
Collaborator

rootulp commented May 26, 2023

Merging b/c @walldiss doesn't have permission to.

@rootulp rootulp merged commit 9efc9bf into celestiaorg:master May 26, 2023
rootulp added a commit that referenced this pull request Jun 6, 2023
This PR contains a breaking change that implements
#201 (comment).
Relevant #200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants