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(system.fileName): file extension count #1101

Merged
merged 12 commits into from Jul 19, 2022

Conversation

xDivisionByZerox
Copy link
Member

Implementation is discussed in #395 (direct comment ref).

This is implemented in a non-breaking manner.

@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Jun 22, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Jun 22, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner June 22, 2022 12:39
@xDivisionByZerox xDivisionByZerox self-assigned this Jun 22, 2022
@xDivisionByZerox
Copy link
Member Author

Since @pkuczynski somewhat provided the implementation in his PR, I want to make them coauthor in this PR. How would this be possible? Is assigning them to the PR enough?

@xDivisionByZerox xDivisionByZerox requested a review from a team June 22, 2022 12:40
@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #1101 (eb94851) into main (316f61f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1101   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files        2146     2146           
  Lines      230438   230462   +24     
  Branches      976      979    +3     
=======================================
+ Hits       229625   229649   +24     
  Misses        792      792           
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/system/index.ts 100.00% <100.00%> (ø)

@prisis
Copy link
Member

prisis commented Jun 22, 2022

Since @pkuczynski somewhat provided the implementation in his PR, I want to make them coauthor in this PR. How would this be possible? Is assigning them to the PR enough?

It is possible if you merge the commit to add a coauthor

src/modules/system/index.ts Outdated Show resolved Hide resolved
@xDivisionByZerox xDivisionByZerox added the needs test More tests are needed label Jun 22, 2022
@xDivisionByZerox
Copy link
Member Author

So by adding new test cases, existing ones (not the new one) are randomly failing? This doesn't seem right to me. I also can reproduce the values in the test by setting the seed to the one in the test.

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 add a seeded test, that uses the new option.

src/modules/system/index.ts Outdated Show resolved Hide resolved
src/modules/system/index.ts Outdated Show resolved Hide resolved
src/modules/system/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Jun 22, 2022

So by adding new test cases, existing ones (not the new one) are randomly failing? This doesn't seem right to me. I also can reproduce the values in the test by setting the seed to the one in the test.

The random seeded tests are less random than one would like. I will try to have a look at the randomness issue soon.
The test previously passing was probably a false positive.

@xDivisionByZerox
Copy link
Member Author

Please add a seeded test, that uses the new option.

Honestly, no. I don't see the point in the seeded tests. No other function has an extra seeded test case for their argument, why would this have? I already added some test cases that check for the argument options in the non-seeded test runs. They are much more valuable IMO.

test/system.spec.ts Outdated Show resolved Hide resolved
test/system.spec.ts Outdated Show resolved Hide resolved
test/system.spec.ts Outdated Show resolved Hide resolved
test/system.spec.ts Outdated Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Jul 4, 2022

The failing test is expecting a common file extension to be included in an array provided in the test. According to our implementation of system.commonFileExt() all of the following would be valid common file types

[
  'gif', 'htm', 'html', 'jpe',
  'jpeg', 'jpg', 'm1v', 'm2a',
  'm2v', 'm3a', 'mp2', 'mp2a',
  'mp3', 'mp4', 'mp4v', 'mpe',
  'mpeg', 'mpg', 'mpg4', 'mpga',
  'pdf', 'png', 'shtml', 'wav',
]

But our expected values are only:

[
  'gif', 'htm', 'html', 'jpeg',
  'm2a', 'm2v', 'mp2', 'mp3',
  'mp4', 'mp4v', 'mpeg', 'mpg',
  'pdf', 'png', 'shtml', 'wav',
]

@ST-DDT
Copy link
Member

ST-DDT commented Jul 5, 2022

Then update the expected values!?
I'm not sure whether I would consider all of them "common".

@xDivisionByZerox
Copy link
Member Author

[...] I would consider all of them "common".

That's where I wasn't sure about and wanted to have some "backup" from you.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 6, 2022

I have rarily/never seen m2a/v mp2 or shtml.
So I would remove them. Lets discuss this tomorrow with everyone.
I miss json and txt.

@xDivisionByZerox xDivisionByZerox force-pushed the feat/system/file-name/provide-file-extension-count branch from 8ead4ef to a76a2d4 Compare July 12, 2022 16:03
@xDivisionByZerox
Copy link
Member Author

For now, I updated the test expectation. In the future we should rework the function to pick extensions from a custom-defined array or something like that.

Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>

Original idea and implementation from Piotr Kuczynski <piotr.kuczynski@gmail.com>
@xDivisionByZerox xDivisionByZerox force-pushed the feat/system/file-name/provide-file-extension-count branch from 4fe3a96 to eb94851 Compare July 19, 2022 08:55
@xDivisionByZerox
Copy link
Member Author

This is non-blocking. Can we merge?

@Shinigami92 Shinigami92 enabled auto-merge (squash) July 19, 2022 09:45
@Shinigami92 Shinigami92 merged commit 968134c into main Jul 19, 2022
@Shinigami92 Shinigami92 deleted the feat/system/file-name/provide-file-extension-count branch July 19, 2022 09:46
hankucz pushed a commit to hankucz/faker that referenced this pull request Jul 19, 2022
Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>
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 needs test More tests are needed p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants