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

Annotate few functions and methods #705

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Conversation

takeda
Copy link
Contributor

@takeda takeda commented Oct 9, 2021

This is related to #491, it doesn't resolve it, but it could be a step into that direction.

This theoretically should not break python 2.7

@takeda takeda marked this pull request as ready for review October 9, 2021 06:57
@takeda takeda force-pushed the types branch 3 times, most recently from 1a00c66 to b9226ff Compare October 9, 2021 07:25
@takeda
Copy link
Contributor Author

takeda commented Nov 10, 2021

@csmarchbanks is there a chance to get this merged? Should I update this patch to resolve current conflicts?

@csmarchbanks
Copy link
Member

Hello, sorry, I missed this one! Thank you for the PR. I am hoping to review #718 this week, and merge it soon. At that point I believe we can support full typing instead of just the annotations. Would it make sense to wait until after #718 is merged?

@takeda
Copy link
Contributor Author

takeda commented Nov 11, 2021

Yeah, that's fine.

@takeda
Copy link
Contributor Author

takeda commented Nov 29, 2021

@csmarchbanks I rebased my code and adapted it to use annotations as 3.6+ is now required. I currently added types that my code is utilizing. This can be used to show how to add types to other parts of the project.

BTW: since types are not used when the code is run and adding typing involves time, the types could be added incrementally over time.

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the update. Generally this is looking pretty good, I left a handful of questions/comments. I completely agree that types should be added incrementally as people use them rather than requiring one big bang commit to add types.

prometheus_client/context_managers.py Show resolved Hide resolved
prometheus_client/metrics.py Show resolved Hide resolved
prometheus_client/metrics.py Outdated Show resolved Hide resolved
prometheus_client/samples.py Outdated Show resolved Hide resolved
Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

One small comment that I missed the first time around, otherwise 👍. Thanks!

@@ -39,6 +39,9 @@
import operator
import re
import sys
from typing import Any, Callable, TypeVar
Copy link
Member

Choose a reason for hiding this comment

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

This file is actually a copy of http://pypi.python.org/pypi/decorator, you can see the file header for more details. That does mean we should not edit it so as to make future upgrades easier.

Copy link
Contributor Author

@takeda takeda Dec 1, 2021

Choose a reason for hiding this comment

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

I removed it, but out of curiosity. Why is this needed and why contextmanager is used?

For example:

class ExceptionCounter:
    def __init__(self, counter: "Counter", exception: Type[BaseException]) -> None:
        self._counter = counter
        self._exception = exception

    def __enter__(self) -> None:
        pass

    def __exit__(self, typ: Optional[Type[BaseException]], value: Optional[BaseException], traceback: Optional[TracebackType]) -> Literal[False]:
        if isinstance(value, self._exception):
            self._counter.inc()
        return False

    def __call__(self, f: "F") -> "F":
        def wrapped(func, *args, **kwargs):
            with self:
                return func(*args, **kwargs)

        return decorate(f, wrapped)

could be (if I understand things correctly):

@contextmanager
def exception_counter(counter: "Counter", exception: Type[BaseException]) -> None:
  try:
    yield
  except exception:
    counter.inc()

Copy link
Member

Choose a reason for hiding this comment

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

You can find the history of the vendored in file in the PR that added it, as well as the linked issue: #91.

I think you probably could restructure the various context managers like you suggest now that we do not support older python versions. I believe that before 3.2 just using @contextmanger would not allow you to use it as a decorator as well so the class was nicer.

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Thanks!

@csmarchbanks csmarchbanks merged commit 822b8e9 into prometheus:master Dec 1, 2021
DMRobertson pushed a commit to matrix-org/synapse that referenced this pull request May 3, 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.

2 participants