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

Refactor urns package #119

Merged
merged 4 commits into from
May 7, 2024
Merged

Refactor urns package #119

merged 4 commits into from
May 7, 2024

Conversation

rowanseymour
Copy link
Member

@rowanseymour rowanseymour commented May 6, 2024

Trying to...

  1. Fix URN Normalization needs to be idempotent #85
  2. Simplify the API (don't have a bunch of type specific things)
  3. For phone numbers.. only do conversion to an international number at parse time and not in Normalize because only at parse time do we typically have the right country code.
  4. Enforce max length of 255 across all schemes

// NewFirebaseURN returns a URN for the passed in firebase identifier
func NewFirebaseURN(identifier string) (URN, error) {
return NewURNFromParts(FCMScheme, identifier, "", "")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

we only created constructors for some schemes.. and we don't need them

Copy link
Contributor

Choose a reason for hiding this comment

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

Those were used in courier but we can update courier to use NewURNFromParts

Copy link
Contributor

Choose a reason for hiding this comment

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

Or as it is most common to not have a query and display for the URN can we have a short cut that we pass the scheme type and the path and that set the query and display to ""?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have added that as urns.New(scheme, path)

@rowanseymour rowanseymour requested a review from norkans7 May 7, 2024 17:11
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 96.12403% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 89.81%. Comparing base (64e21d5) to head (cd756a1).
Report is 1 commits behind head on main.

Files Patch % Lines
urns/urns.go 84.84% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
- Coverage   90.35%   89.81%   -0.55%     
==========================================
  Files          41       42       +1     
  Lines        1991     1954      -37     
==========================================
- Hits         1799     1755      -44     
- Misses        154      160       +6     
- Partials       38       39       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@norkans7 norkans7 left a comment

Choose a reason for hiding this comment

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

Looks good, we could also update the go version to 1.22

@rowanseymour rowanseymour merged commit 637081f into main May 7, 2024
5 checks passed
@rowanseymour rowanseymour deleted the urns_refactor branch May 7, 2024 18:37
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URN Normalization needs to be idempotent
2 participants