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 opentracing-types to dev dependencies #287

Merged
merged 7 commits into from
Dec 8, 2021
Merged

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Dec 6, 2021

As the title states, pulls in types for opentracing from opentracing-types. Has the added bonus of bumping the mypy precision rate to above 80%.

@H-Shay H-Shay requested a review from a team as a code owner December 6, 2021 19:44
DMRobertson
DMRobertson previously approved these changes Dec 6, 2021
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

LGTM!

sygnal/sygnal.py Outdated
@@ -153,7 +153,7 @@ def __init__(
scope_manager=AsyncioScopeManager(),
)

self.tracer = jaeger_cfg.initialize_tracer()
self.tracer = cast(Tracer, jaeger_cfg.initialize_tracer())
Copy link
Contributor

Choose a reason for hiding this comment

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

But having said that, any idea why this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly not entirely! I know in a previous conversation, there was discussion about jaeger.Tracer inheriting from Tracer: #275 (comment) but mypy doesn't seem to be picking that up and in a fit of pique I figured this was a pretty safe cast to make while fully expecting someone to call me on it.

Copy link
Contributor

@reivilibre reivilibre Dec 7, 2021

Choose a reason for hiding this comment

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

sygnal/sygnal.py:156: error: Incompatible types in assignment (expression has type "Optional[jaeger_client.tracer.Tracer]", variable has type "opentracing.tracer.Tracer")  [assignment]

I think the problem is that it's Optional.

Clicking through to the function, I see:

    def initialize_tracer(self, io_loop: Optional[Any] = None) -> Optional[Tracer]:
        """
        Initialize Jaeger Tracer based on the passed `jaeger_client.Config`.
        Save it to `opentracing.tracer` global variable.
        Only the first call to this method has any effect.
        """

        with Config._initialized_lock:
            if Config._initialized:
                logger.warning('Jaeger tracer already initialized, skipping')
                return None
            Config._initialized = True

        tracer = self.new_tracer(io_loop)

        self._initialize_global_tracer(tracer=tracer)
        return tracer

I think my preferred approach would be to assert that it's not None. (I guess you have to do that before assigning it to the field so it's a little clunky, but I think less evil than a full on cast)

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 thought that at first but when I added this code:

tracer = jaeger_cfg.initialize_tracer()
assert tracer is not None
self.tracer = tracer

the error persisted, which is why I thought it might be an issue with the inheritance not being clear to mypy.

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 that's because tracer already exists as a var with a different type annotation.

                    tr = jaeger_cfg.initialize_tracer()
                    assert tr is not None
                    self.tracer = tr

seems to work locally, though probably give it a nicer name :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, I have fixed it.

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

thank you! :)

@H-Shay H-Shay merged commit 1d5c680 into main Dec 8, 2021
@H-Shay H-Shay deleted the check_imported_types branch December 8, 2021 18:11
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.

3 participants