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

Simplify mkdirP implementation #444

Merged
merged 1 commit into from
Apr 28, 2021
Merged

Simplify mkdirP implementation #444

merged 1 commit into from
Apr 28, 2021

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented May 3, 2020

less code = fewer bugs ☺️

The recursive option have been available since Node.js 10.12.0 (ref), and since GitHub Actions runs on Node.js 12.x this should always be available.

@LinusU
Copy link
Contributor Author

LinusU commented May 3, 2020

(force pushed a lint fix)

@actions actions deleted a comment May 6, 2020
@LinusU
Copy link
Contributor Author

LinusU commented May 18, 2020

ping @damccorm, would you be able to review this? ☺️

@damccorm
Copy link
Contributor

@hross could you take a look?

@bryanmacfarlane
Copy link
Member

I would also like to point out that while more code, the current code has been working in azure pipelines and github for 5 years+. changing the code can also introduce bugs and subtle behavioral differences ;). We should confirm 100% that the error case behaviors are the exact same. e.g. see the note about optimistic behaviors of the existing function. If needed, we should introduce more tests to prove the behaviors are the same in all cases.

@bryanmacfarlane bryanmacfarlane requested a review from thboop May 18, 2020 13:12
@bryanmacfarlane
Copy link
Member

@thboop for thoughts.

@LinusU
Copy link
Contributor Author

LinusU commented May 18, 2020

I would also like to point out that while more code, the current code has been working in azure pipelines and github for 5 years+. changing the code can also introduce bugs and subtle behavioral differences ;)

Fair point, although the built-in one have been shipped with every copy of Node.js since 2018, so it has probably been quite battle tested itself ☺️

e.g. see the note about optimistic behaviors of the existing function

Node's code also tries to mkdir the full path first, and then traverses up one step at a time:

https://github.com/nodejs/node/blob/d3a8a23089af06bb047bf9bad7531fbfc70f6314/src/node_file.cc#L1304-L1339

If needed, we should introduce more tests to prove the behaviors are the same in all cases.

I'd be happy to help with this if there is any specific behaviours that you think needs testing 👨‍💻

Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for the contribution! @LinusU

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants