Skip to content

feat: FakerError#718

Merged
ST-DDT merged 15 commits intofaker-js:mainfrom
xDivisionByZerox:707-common-faker-js-error
Mar 31, 2022
Merged

feat: FakerError#718
ST-DDT merged 15 commits intofaker-js:mainfrom
xDivisionByZerox:707-common-faker-js-error

Conversation

@xDivisionByZerox
Copy link
Copy Markdown
Member

closes #707

@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner March 28, 2022 21:23
@xDivisionByZerox xDivisionByZerox self-assigned this Mar 28, 2022
@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent labels Mar 28, 2022
@xDivisionByZerox xDivisionByZerox changed the title 707 common faker js error feat: FakerJsError Mar 28, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team March 28, 2022 21:43
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 28, 2022

Codecov Report

Merging #718 (7d6001a) into main (48dcec1) will decrease coverage by 0.00%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
- Coverage   99.34%   99.34%   -0.01%     
==========================================
  Files        1924     1925       +1     
  Lines      177010   177024      +14     
  Branches      908      909       +1     
==========================================
+ Hits       175858   175871      +13     
- Misses       1096     1097       +1     
  Partials       56       56              
Impacted Files Coverage Δ
src/index.ts 95.00% <0.00%> (-2.44%) ⬇️
src/errors/faker-error.ts 100.00% <100.00%> (ø)
src/fake.ts 100.00% <100.00%> (ø)
src/faker.ts 100.00% <100.00%> (ø)
src/finance.ts 99.30% <100.00%> (+<0.01%) ⬆️
src/mersenne.ts 100.00% <100.00%> (ø)
src/random.ts 99.46% <100.00%> (+<0.01%) ⬆️
src/utils/unique.ts 95.23% <100.00%> (+0.07%) ⬆️

Comment thread src/internal/faker-js.error.ts Outdated
Comment thread src/internal/faker-js.error.ts Outdated
Comment thread test/unique.spec.ts Outdated
Comment thread src/internal/faker.error.ts Outdated
@Shinigami92 Shinigami92 changed the title feat: FakerJsError feat: FakerError Mar 29, 2022
Comment thread src/errors/faker.error.ts Outdated
Comment thread test/unique.spec.ts Outdated
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Mar 29, 2022
Copy link
Copy Markdown
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.

Please re-export FakerError in src/index.ts

ST-DDT
ST-DDT previously approved these changes Mar 29, 2022
Comment thread src/errors/faker.error.ts Outdated
Comment thread src/errors/faker.error.ts Outdated
Comment thread src/fake.ts
pkuczynski
pkuczynski previously approved these changes Mar 30, 2022
@xDivisionByZerox
Copy link
Copy Markdown
Member Author

Please re-export FakerError in src/index.ts

How are things exported in the root index.ts? Is there any import/export order?
Should there be an index file in the errors directory? IMO this would make sense. If yes, would I make implicit or explicit exports in the root index file?

@Shinigami92
Copy link
Copy Markdown
Member

@xDivisionByZerox You just want to do in src/index.ts

import { FakerError } from './errors/faker-error';
// ...
export { FakerError };

@xDivisionByZerox
Copy link
Copy Markdown
Member Author

@xDivisionByZerox You just want to do in src/index.ts

import { FakerError } from './errors/faker-error';
// ...
export { FakerError };

But you know that this can be simplified, do you?

export{ FakerError } from './errors/faker-error';

That's why I was asking because I have seen these redundant import => export statements...

@Shinigami92
Copy link
Copy Markdown
Member

But you know that this can be simplified, do you?

export{ FakerError } from './errors/faker-error';

That's why I was asking because I have seen these redundant import => export statements...

Yeah I know, but I'm not yet used to it

@pkuczynski
Copy link
Copy Markdown
Member

export{ FakerError } from './errors/faker-error';

I personally actually prefer this form, as it's shorter and cleaner what comes from where...

@pkuczynski
Copy link
Copy Markdown
Member

The export is still missing @xDivisionByZerox right?

Copy link
Copy Markdown
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.

index export still missing

@xDivisionByZerox
Copy link
Copy Markdown
Member Author

Yes the export was still missing but is implemented now.

Comment thread src/errors/faker-error.ts
@ST-DDT ST-DDT merged commit c3be3b1 into faker-js:main Mar 31, 2022
@xDivisionByZerox xDivisionByZerox deleted the 707-common-faker-js-error branch March 31, 2022 20:46
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.

Common FakerJSError

4 participants