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

refactor: convert mersenne into a class #530

Merged
merged 6 commits into from
Feb 23, 2022
Merged

refactor: convert mersenne into a class #530

merged 6 commits into from
Feb 23, 2022

Conversation

xDivisionByZerox
Copy link
Member

Created in relation to #280.

Converted MersenneTwister19937 function to a js class declaration.

Things I want to change but need approval:

  • proper camelcase for all methods and variables/properties
  • remove the file scoped variable dbg in line 54 since it is not used anywhere
  • property jsdocs for public methods

@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner February 21, 2022 17:08
@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Feb 21, 2022
@Shinigami92 Shinigami92 linked an issue Feb 21, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #530 (2113fee) into main (03c3d11) will increase coverage by 0.00%.
The diff coverage is 95.70%.

❗ Current head 2113fee differs from pull request most recent head 84c14f0. Consider uploading reports for the commit 84c14f0 to get more accurate results

@@           Coverage Diff           @@
##             main     #530   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1919     1919           
  Lines      176315   176364   +49     
  Branches      904      903    -1     
=======================================
+ Hits       175158   175208   +50     
+ Misses       1101     1100    -1     
  Partials       56       56           
Impacted Files Coverage Δ
src/vendor/mersenne.ts 95.90% <95.59%> (+1.02%) ⬆️
src/mersenne.ts 100.00% <100.00%> (ø)

@xDivisionByZerox
Copy link
Member Author

@Shinigami92 can you confirm that dbg in line 54 is unused and can be removed?
It gets assigned in line 205 but doesn't do anything.
I have a second branch on my fork where I removed it and all test still work fine.

@xDivisionByZerox
Copy link
Member Author

I guess I fked up the method scoping (?). I'm gonna investigate this.

@Shinigami92
Copy link
Member

dbg

Yes, it could be that this is not used, BUT maybe it is something like a global weird thing that does any magic sideeffect 😬
If not, please purge it with fire 🔥

src/vendor/mersenne.ts Outdated Show resolved Hide resolved
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.

Had a first quick look and LGTM, will approve later when I review deeper
Maybe you will also like to improve some stuff

@xDivisionByZerox
Copy link
Member Author

Yes, it could be that this is not used, BUT maybe it is something like a global weird thing that does any magic sideeffect 😬
If not, please purge it with fire 🔥

That's why I was asking if you could checkout my branch where I removed it since on my machine it works fine after removing. Or is there a way to trigger the CI pipeline manually on a specific branch?

@Shinigami92
Copy link
Member

Or is there a way to trigger the CI pipeline manually on a specific branch?

The CI pipeline will be triggered when you open a PR. Just do that and we can close the PR if not needed.

@xDivisionByZerox
Copy link
Member Author

From the CI results in #531 I can confirm that removing the file scoped variable dbg does not influence the results of any tests.

Will remove it now add the removal to this PR.

Shinigami92
Shinigami92 previously approved these changes Feb 23, 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.

Not sure why the codecov/patch hits so hard here, but the rewrite looks good to me 🤷

@Shinigami92 Shinigami92 requested a review from a team February 23, 2022 13:16
ST-DDT
ST-DDT previously approved these changes Feb 23, 2022
@ST-DDT ST-DDT requested a review from a team February 23, 2022 13:20
@Shinigami92
Copy link
Member

Uhm... 👀
Somehow github lost the author when I pressed the rebase button 😯

I don't want to steel credits for that.
@xDivisionByZerox Could you jump into your source and do following?:

  1. Update your main branch in your fork git switch main && git pull upstream (maybe GitHub can help with that)
  2. switch into this branch in your fork git switch refactor/mersenne-twister-to-class
  3. git pull --rebase
  4. git rebase -i main
  5. git push -f

@xDivisionByZerox
Copy link
Member Author

I'm currently at work. Will fix this afternoon.

@xDivisionByZerox
Copy link
Member Author

I'm just getting Everything up-to-date

@ST-DDT
Copy link
Member

ST-DDT commented Feb 23, 2022

@Shinigami92 Everything looks good to me:
grafik
(The gui doesn't show the right avatars though)

@xDivisionByZerox
Copy link
Member Author

Cool. That worked.

@Shinigami92 Shinigami92 merged commit 23a262a into faker-js:main Feb 23, 2022
@xDivisionByZerox xDivisionByZerox deleted the refactor/mersenne-twister-to-class branch February 23, 2022 19:40
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert MersenneTwister19937 to a class
3 participants