Handle reference to undefined array in IPC::Run::run() #164
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
|
Ok, I did not expect those test failures. I will look into this tomorrow. |
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.
Fixes #162.
This pull request currently contains 2 changes:
croakon empty (or undefined) strings in_search_pathcroakon references to undefined arrays inharnessAny of the two would be sufficient on its own to solve this specific problem, but I suggest to go with both.
I think the first change is important because regardless of this issue, it is semantically incorrect to search for empty file names on a path. And the second change is important because if we know we are going to fail, it's better to fail fast. Of course, failing really fast would mean the very first line of
run, but after studying the code a little, it felt like the correct place to handle such special cases is inharness.Instead of failing silently I'm using
croakin both cases, but I'm unsure if this is the correct decision, and would like to hear your input. I'm unsure, because this library has many users, and it is quite possible that someone has accidentally (or not) written code that depends onrunnot failing on undefined values. This fix would break such code. On the other hand, if your code accidentally depends on a bug, maybe you want it to break, so you notice the problem.I would add some tests, but to be perfectly honest, I have no idea how the test framework works.