-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: add string comparison support #99
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Charles-Edouard Brétéché <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
+ Coverage 88.51% 90.81% +2.29%
==========================================
Files 15 15
Lines 2594 2220 -374
==========================================
- Hits 2296 2016 -280
+ Misses 204 112 -92
+ Partials 94 92 -2 ☔ View full report in Codecov by Sentry. |
@springcomp do we have a test suite for that ? |
Signed-off-by: Charles-Edouard Brétéché <[email protected]>
} | ||
// TODO: don't we want to return an error here ? |
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.
@springcomp WDYT about returning an error here ?
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.
My understanding is that this is already covered in the original code by returning nil, nil
.
As this is a non-standard extension, I think we should return that as well if types do not match.
I like the attempt to cast as numbers and compare that.
This means that comparing number-like strings will produce the correct output.
Please, be aware that comparing strings may still exhibit confusing output.
For instance the following two strings will not be considered equal:
élément
(é
LATIN SMALL LETTER E WITH ACUTE ACCENT (U+00E9), …élément
(e
LATIN SMALL LETTER E (U+0065),◌́
COMBINING ACUTE ACCENT (U+0301), …
} | ||
// TODO: don't we want to return an error here ? |
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.
My understanding is that this is already covered in the original code by returning nil, nil
.
As this is a non-standard extension, I think we should return that as well if types do not match.
I like the attempt to cast as numbers and compare that.
This means that comparing number-like strings will produce the correct output.
Please, be aware that comparing strings may still exhibit confusing output.
For instance the following two strings will not be considered equal:
élément
(é
LATIN SMALL LETTER E WITH ACUTE ACCENT (U+00E9), …élément
(e
LATIN SMALL LETTER E (U+0065),◌́
COMBINING ACUTE ACCENT (U+0301), …
I think it's a good first step but we definitely need a conformance test suite IMHO.
What do you mean by non-standard ? |
In the specification, ordering operators are only specified for numbers. A library is free (and more than welcome) to include non-standard features or improvement of course. |
@eddycharly I think this is in good share. |
Add string comparison support.
Fixes #96