Skip to content

Extract Process.Temp#3521

Merged
NathanReb merged 2 commits intoocaml:masterfrom
NathanReb:tmp-file-mgmt
Jun 4, 2020
Merged

Extract Process.Temp#3521
NathanReb merged 2 commits intoocaml:masterfrom
NathanReb:tmp-file-mgmt

Conversation

@NathanReb
Copy link
Copy Markdown
Collaborator

@NathanReb NathanReb commented Jun 2, 2020

This is a first step towards #3432 and is required to move forward with #3392.

@NathanReb NathanReb requested a review from a user June 2, 2020 11:21
@NathanReb NathanReb requested review from nojb and rgrinberg June 2, 2020 11:22
nojb
nojb approved these changes Jun 2, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM.

Would it be useful if it went into stdune instead?

@NathanReb
Copy link
Copy Markdown
Collaborator Author

NathanReb commented Jun 2, 2020

You're right, this bit could actually be moved to stdune. I didn't put it there because the remaining points in #3432 seem a bit specific to dune. Either way works for me!

@NathanReb
Copy link
Copy Markdown
Collaborator Author

NathanReb commented Jun 2, 2020

We could have Stdune.Temp and Dune_temp modules, the latter using the former.

@nojb
Copy link
Copy Markdown
Collaborator

nojb commented Jun 2, 2020

We could have Stdune.Temp and Dune_temp modules, the latter using the former.

Sounds OK to me, but just in case a second opinion would be nice.

@ghost
Copy link
Copy Markdown

ghost commented Jun 2, 2020

To be fair, putting all temporary files into a single temporary directory seems like a good practice to me. So having a single module that lives in Stdune seems fine to me.

@NathanReb
Copy link
Copy Markdown
Collaborator Author

NathanReb commented Jun 2, 2020

Yeah makes sense, I'll move it!

@NathanReb
Copy link
Copy Markdown
Collaborator Author

NathanReb commented Jun 2, 2020

Done!

@NathanReb NathanReb mentioned this pull request Jun 2, 2020
Merged
ghost
ghost approved these changes Jun 2, 2020
View reviewed changes
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good!

I just added a bit of doc

@NathanReb
Copy link
Copy Markdown
Collaborator Author

NathanReb commented Jun 3, 2020

Can I go ahead and merge this despite the CI error?

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Jun 3, 2020

@NathanReb yeah, this is good to merge. The error is unrelated.

NathanReb and others added 2 commits June 4, 2020 11:09
@NathanReb
Extract Process.Temp
30bcdef 
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
@jeremiedimino @NathanReb
doc
901ba7a 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@NathanReb NathanReb merged commit 2681368 into ocaml:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nojb nojb nojb approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@NathanReb @nojb @rgrinberg @jeremiedimino