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(number): add romanNumeral method #3070

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

AmaanRS
Copy link

@AmaanRS AmaanRS commented Aug 22, 2024

This is a draft, please check this

@AmaanRS AmaanRS requested a review from a team as a code owner August 22, 2024 06:43
Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fb538f4
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66e2c2183bfd4b0008f1ea89
😎 Deploy Preview https://deploy-preview-3070.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Shinigami92 Shinigami92 marked this pull request as draft August 22, 2024 15:00
@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent m: number Something is referring to the number module labels Aug 24, 2024
@import-brain import-brain added this to the v9.x milestone Aug 24, 2024
Copy link
Member

@import-brain import-brain left a comment

Choose a reason for hiding this comment

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

Please add tests for romanNumeral

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@AmaanRS
Copy link
Author

AmaanRS commented Aug 28, 2024

In this commit 7abd2c7 i created a new function intToRomanNumeral so where should i place it ?

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@AmaanRS
Copy link
Author

AmaanRS commented Aug 29, 2024

How should i write random seeded test for "it('should generate a Roman numeral between 1 and 10')" ? Since i don't what answer will be.

@AmaanRS
Copy link
Author

AmaanRS commented Aug 29, 2024

And there will be no value range test ?

@matthewmayer
Copy link
Contributor

There's a few ways you could do it. For 1 to 10 you could check if it's any of the values from I to X by listing all 10 values.

You could also check it only contains suitable characters.

Also add tests that check hitting each of the errors and check appropriate error is thrown.

@AmaanRS
Copy link
Author

AmaanRS commented Aug 30, 2024

Please let me know if there are any changes with the code and test if not i will run preflight and then commit the code

@matthewmayer
Copy link
Contributor

Ideally please run preflight after every change. This will help catch any minor syntax errors etc so the reviewers can focus on the logic of your code rather than nitpicking.

src/modules/number/index.ts Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
test/modules/number.spec.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@AmaanRS
Copy link
Author

AmaanRS commented Sep 1, 2024

I ran into an error during preflight FakerApiDocsProcessingError: Failed to process parameter options at src/modules/number/index.ts:468:16 : Expected exactly one element for jsdocs, got 0

@matthewmayer
Copy link
Contributor

Expected exactly one element for jsdocs

its not a very intuitive error message but i think the problem is that you're missing a @since JSDoc tag

add

@since 9.1.0 at the bottom of the JSDoc as this likely wont be in the 9.0.0 release.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 1, 2024

I ran into an error during preflight FakerApiDocsProcessingError: Failed to process parameter options at src/modules/number/index.ts:468:16 : Expected exactly one element for jsdocs, got 0

The error is not caused by @since. It is caused by the options.min/max parameter not having jsdocs.
I'll improve the error message.

FakerApiDocsProcessingError: Failed to process property 'options.min' at src/modules/number/index.ts:467:36 : Expected exactly one element for jsdocs, got 0

The options object should be defined like this:

  romanNumeral(
    options:
      | number
      | {
          /**
           * Lower bound for generated number.
           *
           * @default 1
           */
          min?: number;
          /**
           * Upper bound for generated number.
           *
           * @default 3999
           */
          max?: number;
        } = {}
  ): string {

@matthewmayer matthewmayer marked this pull request as ready for review September 3, 2024 01:09
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (17f570e) to head (fb538f4).
Report is 7 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3070   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files        2776     2776           
  Lines      226260   226301   +41     
  Branches      592      596    +4     
=======================================
+ Hits       226199   226246   +47     
+ Misses         61       55    -6     
Files with missing lines Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
matthewmayer
matthewmayer previously approved these changes Sep 11, 2024
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 m: number Something is referring to the number module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants