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 missing functions/classes to __all__ #757

Merged

Conversation

Pliner
Copy link
Contributor

@Pliner Pliner commented Jan 27, 2022

Hi,

mypy==0.931.0 raises the following errors for prometheus-client==0.13.0:

image

Probably the reason of it are two things:

  1. Change to imports to fix go-to-declaration in editors #747, https://github.com/prometheus/client_python/pull/747/files#diff-719659a170701715fc787a12f9ceb16910e3d9a5fbe2c5725f91fac76b29f0f1L10, https://github.com/prometheus/client_python/pull/747/files#diff-719659a170701715fc787a12f9ceb16910e3d9a5fbe2c5725f91fac76b29f0f1L11
  2. As I know mypy relies on __all__ declaration and I have no idea why it worked before.

I hope it is fine to include these two things to __all__, because of their wider usage compared to other classes/constants mentioned in #747.

Thanks.

Signed-off-by: Yury Pliner <yury.pliner@gmail.com>
@Pliner Pliner force-pushed the add-collector-registry-and-registry-back branch from d8d8f0c to 0c66ee1 Compare January 27, 2022 18:33
@Pliner
Copy link
Contributor Author

Pliner commented Jan 27, 2022

@csmarchbanks Could you look on it please?

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!

If this is an issue for you with those two functions, it will probably be an issue for others with some of the other functions, would you be willing to just add them all?

@csmarchbanks
Copy link
Member

csmarchbanks commented Jan 27, 2022

Also, it would be nice if this would fail in our linter, perhaps as part of this PR you could add implicit_reexport = False to the mypy config to avoid this happening in the future?

Signed-off-by: Yury Pliner <yury.pliner@gmail.com>
@Pliner Pliner force-pushed the add-collector-registry-and-registry-back branch 2 times, most recently from 9253a31 to 8122061 Compare January 28, 2022 09:11
Signed-off-by: Yury Pliner <yury.pliner@gmail.com>
@Pliner Pliner force-pushed the add-collector-registry-and-registry-back branch 2 times, most recently from e9d1446 to c7e9c19 Compare January 28, 2022 10:06
prometheus_client/__init__.py:7: error: Module "prometheus_client.exposition" does not explicitly export attribute "make_asgi_app"; implicit reexport disabled

Signed-off-by: Yury Pliner <yury.pliner@gmail.com>
@Pliner Pliner force-pushed the add-collector-registry-and-registry-back branch from c7e9c19 to e75aa84 Compare January 28, 2022 10:13
@Pliner Pliner changed the title Add CollectorRegistry&REGISTRY to __init__.py Add missing functions/classes to __all__ in __init__.py Jan 28, 2022
@Pliner Pliner changed the title Add missing functions/classes to __all__ in __init__.py Add missing functions/classes to __all__ Jan 28, 2022
@Pliner
Copy link
Contributor Author

Pliner commented Jan 28, 2022

@csmarchbanks Done.

Thanks for the fast feedback!

@@ -17,6 +17,21 @@
from .registry import REGISTRY
from .utils import floatToGoString

__all__ = (
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'm a bit unsure about this, but had to explicitly export in this way because of make_asgi_app.

Copy link
Contributor Author

@Pliner Pliner Jan 28, 2022

Choose a reason for hiding this comment

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

prometheus_client/__init__.py:7: error:Module "prometheus_client.exposition" does not explicitly export attribute "make_asgi_app"; implicit reexport disabled

Copy link
Member

Choose a reason for hiding this comment

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

This seems fine to me, it makes it very clear that those are the things we are exporting.

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!

@@ -17,6 +17,21 @@
from .registry import REGISTRY
from .utils import floatToGoString

__all__ = (
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine to me, it makes it very clear that those are the things we are exporting.

@csmarchbanks csmarchbanks merged commit b3271a3 into prometheus:master Jan 28, 2022
@Pliner Pliner deleted the add-collector-registry-and-registry-back branch January 28, 2022 15:19
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.

None yet

2 participants