This repository was archived by the owner on Apr 14, 2021. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Allow OTP code to be passed through bundler #7005
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if we could fix this problem in the
shmethod rather then just have the release task do its own output/input.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to push this into
sh, I just wasn't sure if you would want to switch all of these functions over to that. Not sure of any other implications it may have.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colby-swandale @kddeisz I had a look at this PR and the related one (#7108), and I tend to think that @kddeisz's approach is better. The fix in #7108 changes all the methods using the
shhelper to print all of there stdout and stderr to the screen. That's quite verbose, specially in the case ofrake build. We might still want to change that and print all those messages to the screen everytime, but for the shake of fixing this particular issue, I would only modify this specific method, which is also the only case where we should actually expect input from the user, I think.What about extracting a
sh_with_inputmethod with the modified logic, and use it only here, for now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gosh yeah sorry this has languished for a while. I can take a look at this again later this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, let's wait for @colby-swandale's input first so that you don't do unnecessary work.