-
-
Notifications
You must be signed in to change notification settings - Fork 932
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
feat(system.fileName): file extension count #1101
Conversation
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? |
Codecov Report
@@ 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
|
It is possible if you merge the commit to add a coauthor |
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. |
There was a problem hiding this 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.
The random seeded tests are less random than one would like. I will try to have a look at the randomness issue soon. |
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. |
0bd31da
to
7028cb7
Compare
7028cb7
to
8ead4ef
Compare
The failing test is expecting a common file extension to be included in an array provided in the test. According to our implementation of [
'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',
] |
Then update the expected values!? |
That's where I wasn't sure about and wanted to have some "backup" from you. |
I have rarily/never seen m2a/v mp2 or shtml. |
8ead4ef
to
a76a2d4
Compare
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>
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
4fe3a96
to
eb94851
Compare
This is non-blocking. Can we merge? |
Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>
Implementation is discussed in #395 (direct comment ref).
This is implemented in a non-breaking manner.