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.

Sign up for GitHub

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

Jump to bottom

Make task.disable_window be default source of window int #3081

Merged
merged 3 commits into from
Aug 22, 2021
Merged

Make task.disable_window be default source of window int #3081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
080d45d
Make task.disable_window be default source of window int
jamescooke May 18, 2021
69dbd59
Merge branch 'master' into patch-1
jamescooke Aug 18, 2021
390f498
Add test on disable_window_seconds
jamescooke Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions luigi/task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,12 @@ def disable_window(self):
Override this positive integer to have different ``disable_window`` at task level.
Check :ref:`scheduler-config`
"""
return self.disable_window_seconds
return None

@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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this line need a test added to get Codecov to go green? Happy to add it if required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what Codecov is complains about. If you can fix it, it would awesome.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added a very basic test on disable_window_seconds which (hopefully) will give the required coverage 🤞🏻


@property
def owner_email(self):
Expand Down
9 changes: 9 additions & 0 deletions test/task_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,15 @@ class MockTimedelta:
with self.assertWarnsRegex(UserWarning, 'Parameter "timedelta_param" with value ".*" is not of type timedelta.'):
DummyTask(**params)

def test_disable_window_seconds(self):
"""
Deprecated disable_window_seconds param uses disable_window value
"""
class ATask(luigi.Task):
disable_window = 17
task = ATask()
self.assertEqual(task.disable_window_seconds, 17)


class ExternalizeTaskTest(LuigiTestCase):

Expand Down