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 doubled extension in system.filePath #300

Merged
merged 6 commits into from
Mar 29, 2022

Conversation

pkuczynski
Copy link
Member

@pkuczynski pkuczynski commented Jan 25, 2022

system.filePath incorrectly creates results with two extensions. This PR is fixing this issue.

Steps to reproduce: faker.system.filePath()
Expected output: '/usr/local/src/money.dotx'
Actual output: '/usr/local/src/money.rmp.dotx'

@pkuczynski pkuczynski requested a review from a team as a code owner January 25, 2022 20:50
@pkuczynski pkuczynski added c: bug Something isn't working p: 3-urgent Fix and release ASAP and removed c: bug Something isn't working labels Jan 25, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 25, 2022

For me it currently looks like you

  1. change the runtime behavior => so 6.1
  2. declare it as p4 but change the comment that explains that it have a double file extension?!

Please do not miss-use the priority labels, this is not a p4, max p3, if this is even a bug at all 🤔

@Shinigami92 Shinigami92 removed the p: 3-urgent Fix and release ASAP label Jan 25, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Jan 25, 2022

if this is even a bug at all 🤔

IMO it's a bug or at least unexpected behavior, but nothing important.

@pkuczynski
Copy link
Member Author

  1. change the runtime behavior => so 6.1

Fixing a bug, so it is kind of "stability", but wasn't sure on your "terminology"

  1. declare it as p4 but change the comment that explains that it have a double file extension?!

I am not sure what do you mean :/

Please do not miss-use the priority labels, this is not a p4, max p3, if this is even a bug at all 🤔

I was not sure if my labels are correct, hence I reached out on discord asking for help with this. I strongly believe it is a bug though.

@pkuczynski pkuczynski changed the title fix: remove duplicated extension in system.filePath fix: remove doubled extension in system.filePath Jan 25, 2022
@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Jan 26, 2022
@Shinigami92
Copy link
Member

Tests are indicating that this was expected 🤔
Maybe we could just provide options and define how many extension will be added 🤔

@pkuczynski
Copy link
Member Author

Tests are indicating that this was expected 🤔
Maybe we could just provide options and define how many extension will be added 🤔

These are the new tests. There were no old tests for this function AFAIK. And I can't really see why anyone would want to have dbl extensions in the filePath (by default)...?

@pkuczynski
Copy link
Member Author

Maybe we could just provide options and define how many extension will be added 🤔

Added as a separate feat PR #395

@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label Mar 15, 2022
@import-brain import-brain added the needs rebase There is a merge conflict label Mar 28, 2022
@pkuczynski pkuczynski removed needs rebase There is a merge conflict s: on hold Blocked by something or frozen to avoid conflicts labels Mar 29, 2022
@pkuczynski
Copy link
Member Author

@Shinigami92 anything else to be done in this PR? I lost the track a little bit?

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #300 (9bae9fc) into main (1bb0f25) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #300   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1924     1924           
  Lines      177021   177022    +1     
  Branches      904      904           
=======================================
+ Hits       175860   175861    +1     
  Misses       1105     1105           
  Partials       56       56           
Impacted Files Coverage Δ
src/system.ts 96.46% <100.00%> (+0.01%) ⬆️

@ST-DDT ST-DDT requested review from a team March 29, 2022 15:54
@ST-DDT ST-DDT merged commit 2532eb9 into faker-js:main Mar 29, 2022
@pkuczynski pkuczynski deleted the system-filepath-dup-ext branch March 29, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants