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: separate methods for object key value #503

Merged
merged 9 commits into from
Apr 30, 2022
Merged

feat: separate methods for object key value #503

merged 9 commits into from
Apr 30, 2022

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Feb 17, 2022

Created in relation to #492.

Thinks I have done:

  • Create and implement 2 new methods faker.random.objectValue and faker.random.objectKey
  • Wrote documentation for both methods
  • Wrote test for both methods
  • Added a deprecation flag and log for faker.random.objectElement

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #503 (f9b8225) into main (6cfd1e1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #503   +/-   ##
=======================================
  Coverage   99.41%   99.41%           
=======================================
  Files        1959     1959           
  Lines      210830   210905   +75     
  Branches      901      906    +5     
=======================================
+ Hits       209600   209675   +75     
  Misses       1172     1172           
  Partials       58       58           
Impacted Files Coverage Δ
src/finance.ts 99.29% <100.00%> (-0.01%) ⬇️
src/helpers.ts 99.32% <100.00%> (+0.02%) ⬆️
src/random.ts 99.54% <100.00%> (+0.03%) ⬆️

ST-DDT
ST-DDT previously approved these changes Feb 17, 2022
src/random.ts Show resolved Hide resolved
@import-brain import-brain added the c: feature Request for new feature label Mar 12, 2022
src/random.ts Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Mar 13, 2022
@ST-DDT ST-DDT requested a review from a team March 13, 2022 13:17
Shinigami92
Shinigami92 previously approved these changes Mar 13, 2022
@xDivisionByZerox xDivisionByZerox changed the title feat: Seperate methods for object key value feat: seperate methods for object key value Mar 25, 2022
src/random.ts Outdated Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member Author

The failing CI doesn't seem to be related to the changes I made, rather than with the fake module.

2022-03-26T09:57:08.4524974Z  > Fake.fake src/fake.ts:85:12
2022-03-26T09:57:08.4525598Z
2022-03-26T09:57:08.4526162Z     if (this.faker[parts[0]] == null) {
2022-03-26T09:57:08.4527269Z           throw new Error('Invalid module: ' + parts[0]);
2022-03-26T09:57:08.4527942Z                             ^
2022-03-26T09:57:08.4528536Z     }

Error: invalid module: {datatype

I guess a rebase could fix this. (?)

@xDivisionByZerox xDivisionByZerox changed the title feat: seperate methods for object key value feat: separate methods for object key value Mar 26, 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.

  1. We need to add test for the deprecation warning
  2. We need to swap out all internal usages like in finance.currencyCode (please use search to find more occurrences)

@Shinigami92
Copy link
Member

The failing CI doesn't seem to be related to the changes I made, rather than with the fake module.

2022-03-26T09:57:08.4524974Z  > Fake.fake src/fake.ts:85:12
2022-03-26T09:57:08.4525598Z
2022-03-26T09:57:08.4526162Z     if (this.faker[parts[0]] == null) {
2022-03-26T09:57:08.4527269Z           throw new Error('Invalid module: ' + parts[0]);
2022-03-26T09:57:08.4527942Z                             ^
2022-03-26T09:57:08.4528536Z     }

Error: invalid module: {datatype

I guess a rebase could fix this. (?)

I'm not totally sure what's going on here, I hope @ST-DDT has some more in-depth inside into the fake function and what could be the problem here related to this PR changes
main branch is green and I did not see this previously somewhere

Here is a local failed test result

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/all_functional.spec.ts > faker.fake functional tests > uk > datatype > array()
Error: Invalid module: {datatype
  Fake.fake src/fake.ts:85:12
     83| 
     84|     if (this.faker[parts[0]] == null) {
     85|       throw new Error('Invalid module: ' + parts[0]);
       |            ^
     86|     }
     87| 
  Fake.fake src/fake.ts:124:16
  test/all_functional.spec.ts:96:35

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]

@ST-DDT
Copy link
Member

ST-DDT commented Mar 26, 2022

I have identified the issue.
The generated string contains a $ which causes the string replace method to insert the token again. In this case the $ was directly after a { which then caused the issue.
I will provide a fix for the faker method.

src/random.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Apr 19, 2022
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Apr 29, 2022
@Shinigami92
Copy link
Member

I will take this PR over so we can merge it soon

@Shinigami92
Copy link
Member

I just found out that we need JSDoc with @deprecated on ALL signatures!

In the one line with not using 'key', it is not strikedthrough...

@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label Apr 29, 2022
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 requested a review from ST-DDT April 29, 2022 13:24
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.

Please also change the helpers module js to something like this:

Module with various helper methods that transform the method input rather than returning values from locales. The transformation process may call methods that use the locale data.

src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 requested a review from ST-DDT April 29, 2022 15:47
@ST-DDT ST-DDT requested a review from a team April 29, 2022 17:25
@Shinigami92 Shinigami92 merged commit 36cd461 into faker-js:main Apr 30, 2022
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 p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add separate methods for random key and value
4 participants