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
Change warning to logger for GSM-7 #382
Conversation
Codecov Report
@@ Coverage Diff @@
## main #382 +/- ##
============================================
- Coverage 78.31% 78.26% -0.06%
- Complexity 1818 1820 +2
============================================
Files 172 172
Lines 4875 4872 -3
============================================
- Hits 3818 3813 -5
- Misses 1057 1059 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Small text change with the error message to be more clear (since it's not really unicode vs ASCII but GSM7 characters)
Couple of places we can tighten up code, and a few other questions
Co-authored-by: Chris Tankersley <chris@ctankersley.com>
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.
LGTM
We've been through several implementations of this, and I think we're at a final one for the behaviour of sending an SMS that can be used by frameworks effectively.
Description
If you have chosen to set your SMS as unicode, it will no longer issue a warning if it contains a GSM-7 charset: you have explicitly told the code that you want to send it as unicode because it is not the default.
If you are sending a message that should be in unicode, but have not changed the type, a log warning will be fired if you have set a logger up for the library. We don't want to stop you sending the message - it might be garbled, but it's your choice to send it this way if you wish. To prevent this, make sure to use the
isGsm7
static method on the SMS object.Motivation and Context
Trying to get the best user experience we can (and it's hard)
How Has This Been Tested?
One test deleted, the other changed to use prophecy to test the call to the logger.
Types of changes
Checklist: