refactor: process handling #8113
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rgrinberg
force-pushed
the
ps/rr/refactor__process_handling
branch
2 times, most recently
from
ab34ea5 to
30c560c
Compare
Refactor process handling. This is has two purposes: * Simplify the implementation and make it easier to follow * Prepare it for making it friendly for action runners At a high level, the refactoring does the following: * Introuce a type for a running process [Process.t] * Introduce a type for the result of running a process [Process.Result.t] * Introduce a type for handling stdout/stderr without accidentally forgetting to clean it up or reading it after they've been deleted. Signed-off-by: Rudi Grinberg <me@rgrinberg.com> <!-- ps-id: f4da9d0f-581a-4fe3-9125-8fa50ab438f8 -->
rgrinberg
force-pushed
the
ps/rr/refactor__process_handling
branch
from
30c560c to
f3bdf77
Compare
Alizter
reviewed
Jul 5, 2023
| } | ||
|
|
||
| module Result = struct | ||
| type nonrec process = t |
dkalinichenko-js
approved these changes
Jul 11, 2023
|
|
||
| type t = | ||
| { on_success : Action_output_on_success.t | ||
| ; mutable unexpected_output : bool |
There was a problem hiding this comment.
I don't like mutable fields here. For now, it's fine because this type is not exposed anywhere in the .mli, but it seems wrong to provide an interface with it.
There was a problem hiding this comment.
Yeah, it's a bit ugly, but I encourage you to understand the old code though :)
We handle 3 separate concerns with mutation:
- We make sure that we never read the file unless we need the output
- Subsequent reads are free
- If we need to swallow the output, we make sure the output we read is always empty.
It also allows us to introduce an optimization:
To check if the output is empty, we don't need to read the file. We can just get its size using stat. That saves an open/close on empty files which should be quite common.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor process handling. This is has two purposes:
At a high level, the refactoring does the following:
[Process.Result.t]
forgetting to clean it up or reading it after they've been deleted.
Signed-off-by: Rudi Grinberg me@rgrinberg.com