feat(string): move methods to new module#1155
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #1155 +/- ##
==========================================
- Coverage 99.63% 99.63% -0.01%
==========================================
Files 2164 2165 +1
Lines 236852 237100 +248
Branches 1003 1015 +12
==========================================
+ Hits 235980 236225 +245
- Misses 851 854 +3
Partials 21 21
|
pkuczynski
left a comment
There was a problem hiding this comment.
Code wise look good, I am not approving only to have a chance to discuss the comments I left...
afa3cf8 to
298cd80
Compare
51cdb39 to
8984e14
Compare
There was a problem hiding this comment.
BLOCKED: we cannot name the module String as this is the JavaScript global String constructor and we would run into the same issues as with _Date module (were we actually also want to find a better name for, like e.g. DateModule or Temporal)
If we name it StringModule please first merge #932
|
I'm not sure whether casing should be case instead. If I google for character case I get 6.390.000.000 results. Could a native English speaker comment whether (char) casing is a real word and which one is the correct one context wise? |
|
I decided against case because in some context it can be conflict with switch-case. If you have another parameter name proposal, please tell us. |
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
ST-DDT
left a comment
There was a problem hiding this comment.
There are some inconsistencies in this PR that needs to be resolved in later PRs as part of #1349 and avoided in future PRs .
e.g. the alphaNumeric function changed its signature during the move, but similar signature adjustments in other methods were moved to new/separate issues.
I only approve this because this is a massive PR, fixing these would make the PR even harder to review than it is already.
It also blocks further development and we have issues to track the remaining todos.
This PR introduces a new module:
String. This was heavily discussed in #805.I did not create any new functions. I just moved a bunch of them into the new module and deprecated the old ones.
Which functions made it to the
Stringmodule?