Fix for #589 - Should be able to transform with default timeout #593
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
|
Since it didn't link in the title - #589 |
|
tfoote
approved these changes
May 15, 2023
I was about to write a test, when I noticed this. The purpose of writing this line was to suppress this very error right? So does just commenting this out work? |
|
You don't want to remove/change that case. The unit tests should validate both cases with and without the dedicated thread. In the case of a dedicated thread both 0 and non-zero timestamps should not error. In the case of no-dedicated thread, the zero timestamp should not error, but if there's a timeout it should error. |
When the timeout is default (0.0), it should NOT error. When the timeout is NOT default, it should raise an error.
|
@clalancette @ahcorde if the added test is alright with you then this can be merged. |
|
ahcorde
approved these changes
Jul 17, 2023
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 discussed in the issue, this is the fix. I haven't written any tests for this, let me know if I have missed out something.