-
Notifications
You must be signed in to change notification settings - Fork 56
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 mnc/mcc fields to Carrier #59
base: main
Are you sure you want to change the base?
Conversation
Should we be bumping versions in merge requests? Is it not more logical to bump them independently? I'm not very good with version management myself though. |
pub struct Carrier { | ||
pub mcc: u16, // always 3 digits | ||
pub mnc: u16, // 2 or 3 digits | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one of the rare occasions where we should prefer getters to fields? We might change the representation over time, e.g. by compressing mcc
and mnc
in a single u16.
Additionally, MNC 001 is not the same as MNC 01, so I'm not sure this representation is the safest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I guess we could make it &str
depending on the lifetime of PhoneNumber
. I'll rework the PR ASAP.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 71.81% 72.16% +0.35%
==========================================
Files 19 19
Lines 1955 1969 +14
==========================================
+ Hits 1404 1421 +17
+ Misses 551 548 -3 ☔ View full report in Codecov by Sentry. |
@gferon any chance you could have a look at this some time soon? Otherwise I might take this up. |
Will be used in whisperfish/libsignal-service-rs#241 instead of having this in
libsignal-service-rs
.