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

Add type annotations #427

Merged
merged 16 commits into from
Feb 11, 2021
Merged

Conversation

MartinThoma
Copy link
Contributor

No description provided.

@MartinThoma MartinThoma force-pushed the type-annotations branch 6 times, most recently from 0f309fd to ddd3676 Compare February 1, 2021 19:30
@coveralls
Copy link

coveralls commented Feb 1, 2021

Coverage Status

Coverage decreased (-0.3%) to 99.663% when pulling c8be406 on MartinThoma:type-annotations into 32a1400 on dbader:master.

Copy link
Collaborator

@SijmenHuizenga SijmenHuizenga left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this @MartinThoma! This looks really good. I love the types and the updated formatting 🚀

schedule/__init__.py Outdated Show resolved Hide resolved
schedule/__init__.py Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@SijmenHuizenga SijmenHuizenga linked an issue Feb 4, 2021 that may be closed by this pull request
dbader and others added 7 commits February 8, 2021 13:40
* Added 'repeat' decorator and written a test. Increased the version by 0.0.1.

* Removed the type of the given job for python 2.* compatibility.

* Added some newlines to comply with FLAKE8

* Added import for 'repeat' in the code example

* Added decorator docs and the ability to pass arguments to the job through the decorator

* Fix FLAKE8

* Added rhagenaars to authors

* Apply suggestions from code review
Partial functions for example do not have a __name__, so they need the
same treatment in __str__ as in __repr__ in order to show a name-like
thing.
* Added conda to installation instructions

* Update installation.rst
@MartinThoma MartinThoma force-pushed the type-annotations branch 3 times, most recently from 32ea7da to 274e622 Compare February 10, 2021 21:54
@MartinThoma
Copy link
Contributor Author

There are 4 errors mentioned by mypy where I'm uncertain if they can be ignored:

schedule/__init__.py:251: error: Item "function" of "Optional[Callable[..., Any]]" has no attribute "args"
schedule/__init__.py:251: error: Item "None" of "Optional[Callable[..., Any]]" has no attribute "args"
schedule/__init__.py:252: error: Item "function" of "Optional[Callable[..., Any]]" has no attribute "keywords"
schedule/__init__.py:252: error: Item "None" of "Optional[Callable[..., Any]]" has no attribute "keywords"

schedule/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@SijmenHuizenga SijmenHuizenga left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few nitpicky comments left.

And let's add a note in the docs about mypy.

Can be returned from a job to unschedule itself.
"""

pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's undo these removals of pass and these formatting changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I undid it. However, I want to point out that the "pass" has absolutely no effect here. It's like a comment. Pass only is necessary if nothing follows, e.g. if the docstring wasn't there.

@@ -157,7 +148,7 @@ def cancel_job(self, job):
except ValueError:
logger.debug('Cancelling not-scheduled job "%s"', str(job))

def every(self, interval=1):
def every(self, interval: int = 1) -> "Job":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def every(self, interval: int = 1) -> "Job":
def every(self, interval: int = 1) -> Job:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will only work with from __future__ import annotations as the class Job is not defined at this point. In Python < 3.8 the annotation is actually evaluated when the signature is parsed. Meaning the class "Job" would have to exist. Or we import the future annotations ... but that will also only do the trick for Python 3.7. There is no way around the quotes for Python 3.6

@@ -167,13 +158,13 @@ def every(self, interval=1):
job = Job(interval, self)
return job

def _run_job(self, job):
def _run_job(self, job: "Job") -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def _run_job(self, job: "Job") -> None:
def _run_job(self, job: Job) -> None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above

self.scheduler = scheduler # scheduler to register with

def __lt__(self, other):
def __init__(self, interval, scheduler: Scheduler = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

add explicit type to interval

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I made it an int :-)

Copy link
Collaborator

@SijmenHuizenga SijmenHuizenga left a comment

Choose a reason for hiding this comment

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

This looks good to go! Really great work @MartinThoma!!!!

@SijmenHuizenga SijmenHuizenga merged commit 76ae16c into dbader:master Feb 11, 2021
@MartinThoma
Copy link
Contributor Author

Thank you very much for all the support for this PR and work on schedule in general 🤗

@MartinThoma MartinThoma deleted the type-annotations branch February 11, 2021 22:30
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.

Type hints
6 participants