Skip to content
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

Allow 0 time timedeltas, fix serialize to allow passing timedelta par… #1968

Merged
merged 4 commits into from
Dec 26, 2016

Conversation

kierkegaard13
Copy link
Contributor

…ams to subtasks

This PR primarily fixes the serialize function for TimeDeltaParameters to allow passing the parameter to a subtask that is required or yielded in the run function. It also allows creating a parameter with 0 time, which helps with setting defaults.

Description

This is to address problems raised in issue #1966

Motivation and Context

This is to address problems raised in issue #1966

Have you tested this? If so, how?

I have included unit tests and tested it working on another project

@mention-bot
Copy link

@kierkegaard13, thanks for your PR! By analyzing the history of the files in this pull request, we identified @eepstein, @Tarrasch and @erikbern to be potential reviewers.

@Tarrasch
Copy link
Contributor

Great job! But I see no tests for the serialization that you fixed. :)

@kierkegaard13
Copy link
Contributor Author

@Tarrasch my bad. I just added them. Hypothesis didn't have a timedelta strategy that I could see, so I wrote some manual test cases if that's okay. Let me know if there are any others you'd like to see.

@Tarrasch
Copy link
Contributor

@kierkegaard13. Looks good. But could you just move out the test to the same parameter_tests.py file? I think there are already tests (without hypothesis) related to serialization in there.

@kierkegaard13
Copy link
Contributor Author

@Tarrasch done.

@kierkegaard13
Copy link
Contributor Author

@Tarrasch think the build stalled. Are you able to restart it, or should I just close and open the PR?

@Tarrasch Tarrasch merged commit 06a7539 into spotify:master Dec 26, 2016
@Tarrasch
Copy link
Contributor

Thanks @kierkegaard13! :)

kreczko pushed a commit to kreczko/luigi that referenced this pull request Mar 28, 2017
This was referenced Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants