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

fix: remove unreachable code finance #508

Merged
merged 2 commits into from
Mar 21, 2022
Merged

fix: remove unreachable code finance #508

merged 2 commits into from
Mar 21, 2022

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Feb 18, 2022

Created in relation to #375

The original issue only stated code for the following if statement:

if (provider in localeFormat) {
    formats = localeFormat[provider]; // there could be multiple formats
    if (typeof formats === 'string') {
        format = formats;
    } else {
        format = this.faker.random.arrayElement(formats);
    }
}

I also found, that the last else statement:

else {
    // Choose a random provider
    // TODO ST-DDT 2022-01-30: #375 This is impossible to access
    if (typeof localeFormat === 'string') {
        format = localeFormat;
    } else if (typeof localeFormat === 'object') {
        // Credit cards are in a object structure
        formats = this.faker.random.objectElement(localeFormat, 'value'); // There could be multiple formats
        if (typeof formats === 'string') {
          format = formats;
        } else {
          format = this.faker.random.arrayElement(formats);
        }
    }
}

has the same problem. So I removed the unreachable parts as well.

@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Merging #508 (50f27d0) into main (9ab0825) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 50f27d0 differs from pull request most recent head 00cd658. Consider uploading reports for the commit 00cd658 to get more accurate results

@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
+ Coverage   99.33%   99.34%   +0.01%     
==========================================
  Files        1920     1919       -1     
  Lines      176486   176318     -168     
  Branches      910      901       -9     
==========================================
- Hits       175308   175164     -144     
+ Misses       1122     1098      -24     
  Partials       56       56              
Impacted Files Coverage Δ
src/finance.ts 100.00% <100.00%> (+0.67%) ⬆️
src/vendor/user-agent.ts 97.75% <0.00%> (-0.29%) ⬇️
src/system.ts 96.37% <0.00%> (-0.11%) ⬇️
src/git.ts 100.00% <0.00%> (ø)
src/date.ts 100.00% <0.00%> (ø)
src/fake.ts 100.00% <0.00%> (ø)
src/iban.ts 100.00% <0.00%> (ø)
src/time.ts 100.00% <0.00%> (ø)
src/word.ts 100.00% <0.00%> (ø)
src/phone.ts 100.00% <0.00%> (ø)
... and 20 more

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.

Somehow the node 14 test fails, could you have a look?

src/finance.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Feb 19, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Feb 19, 2022

FFR:
grafik

@ST-DDT
Copy link
Member

ST-DDT commented Feb 19, 2022

@Shinigami92 I assume that test is wrong.

faker/test/random.spec.ts

Lines 180 to 187 in 431c108

it('should be able to ban some characters', () => {
const alphaText = faker.random.alphaNumeric(5, {
bannedChars: ['a', 'p'],
});
expect(alphaText).toHaveLength(5);
expect(alphaText).match(/[b-oq-z]/);
});

alphaNumeric(

It just has a very low chance of failing (~0.2%).

The test should be fixed like this:

- expect(alphaText).match(/[b-oq-z]/);
+ expect(alphaText).match(/^[0-9b-oq-z]{5}$/);

The same is true for the following test.
I also recommend to ban more chars, as the current test only has a chance of about ~25%(?) to actually the test the expected result.

ST-DDT
ST-DDT previously approved these changes Feb 19, 2022
@ST-DDT ST-DDT requested a review from a team February 19, 2022 12:13
@ST-DDT ST-DDT added this to the v6.1 - First bugfixes milestone Feb 19, 2022
Shinigami92
Shinigami92 previously approved these changes Mar 10, 2022
@Shinigami92 Shinigami92 requested a review from a team March 10, 2022 19:36
@Shinigami92 Shinigami92 added the c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs label Mar 15, 2022
Leyla Jähnig and others added 2 commits March 21, 2022 14:37
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Mar 21, 2022
@ST-DDT ST-DDT requested review from Shinigami92 and a team March 21, 2022 13:38
@Shinigami92 Shinigami92 enabled auto-merge (squash) March 21, 2022 13:40
@Shinigami92 Shinigami92 merged commit 1bc622a into faker-js:main Mar 21, 2022
@xDivisionByZerox xDivisionByZerox deleted the fix/unreachable-code-finance-credit-card-number branch April 15, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unreachable code in finance.creditCardNumber()
4 participants