Allow repeated invocation #37
Merged
Conversation
This file contains hidden or 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
As noted in the comment, repeated invocations of this action might have caused environment variables to overflow. This cute hack avoid this, allowing to reconfigure environment.
While this approach mostly works, I still have reservations so let's leave some cautionary notes.
Well, let's build our "Hello, world!" four times, for four different architectures, because why not.
pzhlkj6612
reviewed
May 8, 2021
Contributor
pzhlkj6612
left a comment
pzhlkj6612
left a comment
There was a problem hiding this comment.
Hi. I have a question about this PR.
Comment on lines
-143
to
+158
| const old_value = old_env_vars[name] | ||
| let [name, new_value] = string.split('=') | ||
| let old_value = old_env_vars[name] |
Contributor
There was a problem hiding this comment.
Hi. The variable old_value will not be changed in each loop, why not keep on using const ?
Owner
Author
There was a problem hiding this comment.
No real reason. I just like pretty columns.
let [name, new_value] = ...
let old_value = ...looks a bit neater than
let [name, new_value] = ...
const old_value = ...Strictly speaking name also is unchanged so this could have been something like
const [name, new_value] = ...
const old_value = ...
let exported_value
if (isPathVariable(name)) {
exported_value = filterPathValue(new_value)
} else {
exported_value = new_value
}or maybe even
const exported_value = maybeFilterPathValue(name, new_value)where maybeFilterPathValue() inspects name and does filtering if necessary.
But this code isn't really that huge so there is no point in trying to enforce some “correct” code style.
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.
As noted in the comment, repeated invocations of this action might have caused environment variables to overflow. This cute hack avoid this, allowing to reconfigure environment.
Resolves: #8 (comment)