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

feat(string): move methods to new module #1155

Merged
merged 49 commits into from
Oct 18, 2022
Merged

Conversation

xDivisionByZerox
Copy link
Member

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 String module?

was now
datatype.string() string.random()
datatype.uuid() string.uuid()
datatype.hexadecimal() string.hexadecimal()
random.alpha() string.alpha()
random.alphaNumeric() string.alphaNumeric()
random.numeric() string.numeric()

@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature s: accepted Accepted feature / Confirmed bug labels Jul 15, 2022
@xDivisionByZerox xDivisionByZerox added this to the v8 - Next Major milestone Jul 15, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner July 15, 2022 21:37
@xDivisionByZerox xDivisionByZerox self-assigned this Jul 15, 2022
@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #1155 (f5e25f8) into next (c977dbc) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

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              
Impacted Files Coverage Δ
src/faker.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/modules/color/index.ts 99.73% <100.00%> (ø)
src/modules/database/index.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 99.12% <100.00%> (-0.06%) ⬇️
src/modules/finance/index.ts 100.00% <100.00%> (ø)
src/modules/git/index.ts 100.00% <100.00%> (ø)
src/modules/random/index.ts 98.98% <100.00%> (-0.37%) ⬇️
src/modules/string/index.ts 100.00% <100.00%> (ø)
src/modules/vehicle/index.ts 100.00% <100.00%> (ø)
... and 1 more

@import-brain import-brain requested a review from a team July 16, 2022 00:20
Copy link
Member

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

Code wise look good, I am not approving only to have a chance to discuss the comments I left...

src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Show resolved Hide resolved
@import-brain import-brain added the needs rebase There is a merge conflict label Jul 22, 2022
@xDivisionByZerox xDivisionByZerox force-pushed the feat/modules/string branch 2 times, most recently from 51cdb39 to 8984e14 Compare July 28, 2022 08:54
@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent and removed needs rebase There is a merge conflict labels Jul 28, 2022
@import-brain import-brain added deprecation A deprecation was made in the PR needs rebase There is a merge conflict labels Aug 11, 2022
src/modules/database/index.ts Outdated Show resolved Hide resolved
src/modules/finance/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 changed the title feat(modules): string feat(string): move methods to new module Aug 12, 2022
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

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

@import-brain import-brain added the s: on hold Blocked by something or frozen to avoid conflicts label Aug 13, 2022
@ST-DDT ST-DDT linked an issue Sep 6, 2022 that may be closed by this pull request
@ST-DDT
Copy link
Member

ST-DDT commented Oct 17, 2022

I'm not sure whether casing should be case instead.
In JS the method is called to lower/upper case as well.

If I google for character case I get 6.390.000.000 results.
If I google for character casing I only get 21.700.000 results.
That's more than two orders of magnitude in difference!

Could a native English speaker comment whether (char) casing is a real word and which one is the correct one context wise?

@Shinigami92
Copy link
Member

I decided against case because in some context it can be conflict with switch-case.
I explicitly want to move away from Parameter names that can collide with JS or TS syntax.

If you have another parameter name proposal, please tell us.

src/modules/random/index.ts Outdated Show resolved Hide resolved
src/modules/vehicle/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
test/random.spec.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
test/string.spec.ts Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Show resolved Hide resolved
@Shinigami92 Shinigami92 added the m: string Something is referring to the string module label Oct 18, 2022
Shinigami92 and others added 5 commits October 18, 2022 08:27
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>
test/string.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
import-brain
import-brain previously approved these changes Oct 18, 2022
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

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.

@Shinigami92 Shinigami92 merged commit 79858fe into next Oct 18, 2022
@Shinigami92 Shinigami92 deleted the feat/modules/string branch October 18, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature deprecation A deprecation was made in the PR m: string Something is referring to the string module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Move String methods into own module
5 participants