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

Use an option for setting mkdirP's max loop size #8

Closed
wants to merge 2 commits into from

Conversation

jclem
Copy link
Contributor

@jclem jclem commented May 22, 2019

  • No need to dirty up the environment
  • Use a jest pattern for expecting the right error/assertion count

- No need to dirty up the environment
- Use a jest pattern for expecting the right error/assertion count
@jclem jclem requested a review from damccorm May 22, 2019 16:35
@damccorm
Copy link
Contributor

damccorm commented May 22, 2019

Is loopMax an option we plan for people to ever use?

If yes, we should probably increase the default to something higher so that configuring it is wholly unnecessary.

If no, it seems weird to me to expose it in the public facing API. I think I'd rather have it as a env variable. If we think that's an issue, I think we should move the implementation into io-helper and still only expose mkdirP(fsPath: string): Promise<void> in io.ts. So that function would look something like:

export async function mkdirP(fsPath: string): Promise<void> {
   return io-util.mkdirP(fsPath, 1000);
}

And we can still test that functionality by directly calling the io-util implementation.

That way nobody takes a dependency on and/or gets confused by loopMax which seems like an implementation detail to me.

@jclem
Copy link
Contributor Author

jclem commented May 22, 2019

Good point, I'll update.

@jclem
Copy link
Contributor Author

jclem commented May 22, 2019

Closed in favor of #9

@jclem jclem closed this May 22, 2019
@damccorm damccorm deleted the features/io-loopmax branch May 22, 2019 21:00
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.

2 participants