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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add zip method #68
feat: Add zip method #68
Changes from 8 commits
6c4cb3b
599bc8a
ee4ab7b
30bc9a2
f1b01b1
c0b50f7
6cf4799
d5790ec
4b25f73
2cb679c
7470944
6707539
7b9c83d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
One thing I'm hung up on is whether or not its better to let whoever is invoking the function to print the error. Printing to console is a user facing behaviour and having a low level utility function do that seems more surprising than just throwing the error.
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.
Oops, added this comment, but forgot to submit the review. Fine to disregard, but still wanted to bring it up.
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 agree that an error thrown by a low-level function may be unexpected. However, given that the zipping behavior is determined by user settings in the sauce configuration, such an error could be understandable and helpful.
It allows users to debug and modify their configuration as necessary. This is different from generating JUnit or Sauce reports, where a failure could be misinterpreted as a test failure, as those operations are not directly controlled by the user.
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.
@tianfeng92 Mike is talking about removing this log line and instead throwing the error all the way back to the caller (the runner in that case). The caller (runner) is then the one to handle (i.e. log) the error.
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.
Now that you bring this up, I'd personally prefer that approach. Generally I'm in favor of keeping user facing messaging out of low level libs. Having user facing messages in low level libs, prevents the lib from being used more broadly and robs the caller of controlling what the user should see.
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.
Make sense. Let me draft a PR to address it.