Rename CentralPlannerScheduler to Scheduler #1781
Merged
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
I find very little value in having that complicated name. In particular it should be clear that the main class of scheduler.py is in fact the Scheduler() class. I've also renamed the tests testing the api to test/scheduler_api_test.py. This should feel more intuitive to have "scheduler" in the name of the file.
| @@ -247,7 +247,7 @@ def run(api_port=8082, address=None, unix_socket=None, scheduler=None, responder | |||
| Runs one instance of the API server. | |||
| """ | |||
| if scheduler is None: | |||
| scheduler = CentralPlannerScheduler() | |||
| scheduler = Scheduler() | |||
There was a problem hiding this comment.
This kind of code is more readable IMO, requires no brain cycles to process anymore.
|
lgtm |
p7k
pushed a commit
to Celmatix/luigi
that referenced
this pull request
Jul 28, 2016
* spotify/master: (24 commits) Add DateSecondParameter to parameter.py (spotify#1779) tox: Specify sphinx dependency better flake8: Unbreak travis build Excludes .tox from flake8 to prevent checking third-party libraries (spotify#1785) README: Remove monthly downloads badge Rename CentralPlannerScheduler to Scheduler (spotify#1781) Remove abstract Scheduler class (spotify#1778) Assistants: Don't affect longevity of tasks (spotify#1772) tests: Skip a inttermittently failing s3 test (spotify#1777) Update retcodes to handle new cases (spotify#1771) tests: Fix warning in remote_scheduler_test.py (spotify#1774) Remove sitecustomize file (spotify#1755) Fix exist method for ftp server Update copy() to return number and size of files copied Remove the confusing "dummy_test_module" directory (spotify#1756) Disable codecov comments on GitHub PRs (spotify#1754) Fix "owner_email" log message. (spotify#1762) docs: Install sphinx 1.4.4 in setup.py (spotify#1761) docs: Set minimum versions for sphinx (spotify#1760) Normalize ListParameter to be Immutable (spotify#1759) ...
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.
Description
Rename CentralPlannerScheduler to Scheduler
Motivation and Context
I find very little value in having that complicated name. In particular
it should be clear that the main class of scheduler.py is in fact
the Scheduler() class.
I've also renamed the tests testing the api to
test/scheduler_api_test.py. This should feel more intuitive to have
"scheduler" in the name of the file
Have you tested this? If so, how?
I rely on Travis for this. I could try this in production before merging to, just in case. :)