Make task.disable_window be default source of window int #3081
Conversation
|
Please could one of @dlstadther , @Tarrasch or another maintainer approve the workflow so that tests run? |
|
|
||
| @property | ||
| def disable_window_seconds(self): | ||
| warnings.warn("Use of `disable_window_seconds` has been deprecated, use `disable_window` instead", DeprecationWarning) | ||
| return None | ||
| return self.disable_window |
There was a problem hiding this comment.
Does this line need a test added to get Codecov to go green? Happy to add it if required.
There was a problem hiding this comment.
I think this is what Codecov is complains about. If you can fix it, it would awesome.
There was a problem hiding this comment.
quick skim looks like only the scheduler's disable_window param is unittested. I don't immediately see any of the Task's. Having test coverage would be great if there's valuable logic that be added to verify. I won't block approval and merge without it. Let me know what you want to do.
There was a problem hiding this comment.
Have added a very basic test on disable_window_seconds which (hopefully) will give the required coverage 🤞🏻
Description
When accessing
task.disable_windowproperty, use the property directly. Do not call the deprecatedtask.disable_window_seconds.Motivation and Context
Fixes #3029
Current implementation gives a deprecation warning even when accessing the correct
task.disable_windowproperty, which is confusing. Fixing this means that the deprecation warning becomes more meaningful - it will only appear whentask.disable_window_secondsis incorrectly accessed.Have you tested this? If so, how?
Have ran this change locally for me in my employers' Luigi test suite and removed 21 warnings when running a small test on a single task. However, that test suite does not use
disable_window.