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

Type annotations #151

Closed
wants to merge 8 commits into from
Closed

Type annotations #151

wants to merge 8 commits into from

Conversation

orsinium
Copy link

@orsinium orsinium commented Nov 2, 2021

There are a few type errors that require carefully changing the code. I decided to not do any drastic code changes just yet. So, adding mypy on CI is something to do in a follow-up PR.

@ghost
Copy link

ghost commented Nov 4, 2021

Looking good to me. I think it'll be great to get mypy in tox.

@orsinium
Copy link
Author

orsinium commented Nov 5, 2021

Done. I've added both mypy and flake8 into tox config. Keep in mind that mypy now produces errors, I propose to fix them later. Also, for that reason, I don't run mypy on CI just yet.

Additionally, I've unlocked the flake8 version constraint since CI always installs the latest release.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Very nice!

csestelo pushed a commit to Destygo/pystatsd that referenced this pull request Jul 18, 2022
)

* Bugfix issue when nodes were not sorted when saved in a flow

* Upgrade requirements
csestelo pushed a commit to Destygo/pystatsd that referenced this pull request Jul 18, 2022
@@ -1,3 +1,4 @@
mock==1.0.1
nose==1.2.1
flake8==1.7.0
flake8
mypy==0.910
Copy link
Contributor

Choose a reason for hiding this comment

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

Upgrade?

@jsocol jsocol added this to the 4.1 milestone Nov 6, 2022
@@ -34,15 +44,21 @@ def timing(self, stat, delta, rate=1):
delta = delta.total_seconds() * 1000.
self._send_stat(stat, '%0.6f|ms' % delta, rate)

def incr(self, stat, count=1, rate=1):
def incr(self, stat: str, count: int = 1, rate: float = 1) -> None:

Choose a reason for hiding this comment

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

I think this might accept non-integer values, at least, there's a test for that case

cl.incr('foo', 1.2)

Suggested change
def incr(self, stat: str, count: int = 1, rate: float = 1) -> None:
def incr(self, stat: str, count: float = 1, rate: float = 1) -> None:

@orsinium
Copy link
Author

orsinium commented Nov 7, 2022

There are now merge conflicts, and I don't work with pystatsd anymore. If anyone is interested in the proposed changes, feel free to pick up the changes from this PR and send a new PR based on it. I'll leave the branch and the fork alive if anyone is interested.

@orsinium orsinium closed this Nov 7, 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.

Type hints & PEP 561 packaging
4 participants