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

Change imports to avoid pylint namespace errors #376

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

c24t
Copy link
Member

@c24t c24t commented Jan 23, 2020

This is 9d3ef06d split off from #282 at @mauriciovasquezbernal's request.

See #282 (comment).

Edit: changed to use pylint:ignore=no-member in the modules failing lint instead. These errors are almost never meaningful, and this prevents us from having to play type inference whack-a-mole in the future if these imports change.

@c24t c24t requested a review from a team January 23, 2020 18:33
@codecov-io
Copy link

Codecov Report

Merging #376 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   85.08%   85.22%   +0.14%     
==========================================
  Files          38       38              
  Lines        1897     1916      +19     
  Branches      225      226       +1     
==========================================
+ Hits         1614     1633      +19     
  Misses        218      218              
  Partials       65       65
Impacted Files Coverage Δ
...src/opentelemetry/ext/opentracing_shim/__init__.py 95.93% <100%> (+0.17%) ⬆️
...app/src/opentelemetry_example_app/flask_example.py 100% <0%> (ø) ⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 90.94% <0%> (+0.06%) ⬆️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py 85.18% <0%> (+0.18%) ⬆️
...ts/src/opentelemetry/ext/http_requests/__init__.py 89.74% <0%> (+0.26%) ⬆️
opentelemetry-sdk/src/opentelemetry/sdk/util.py 85.88% <0%> (+0.34%) ⬆️
...opentelemetry/sdk/context/propagation/b3_format.py 87.27% <0%> (+0.48%) ⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py 68.18% <0%> (+0.58%) ⬆️
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py 70.83% <0%> (+2.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccb97e5...01619bc. Read the comment docs.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

I tried it on top of #282 (after reverting the previous changes to avoid these errors) and it works good.

@Oberon00
Copy link
Member

no-member is IMHO often a very useful warning, so it's a pity that it has false positives here. Do we know the reason? I think we can merge this as a quick & dirty workaround to unblock us, but this should really be resolved. Maybe we need to check the tests in separate pylint invocations each, to not get name clashes that confuse pylint (similar to the http_requests module name debacle #94).

@c24t
Copy link
Member Author

c24t commented Jan 28, 2020

Maybe we need to check the tests in separate pylint invocations each, to not get name clashes that confuse pylint (similar to the http_requests module name debacle #94).

I think that's exactly what's happening. On 5a224ad pylint ext/opentelemetry-ext-opentracing-shim/**/*.py passes as expected, but the big omnibus pylint invocation fails because (evidently) it's picking up the opentracing module in examples/ instead.

Worse, it looks like this happens on class name collisions too. AFAICT lint fails on fec0fbe because test_shim.py does from opentracing import Span, but an earlier import imports Span from opentelemetry.trace. One more argument for importing modules instead of classes.

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

@c24t c24t merged commit 4674973 into open-telemetry:master Jan 29, 2020
@c24t c24t deleted the ot-shim-import-changes branch January 29, 2020 21:31
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Feb 17, 2020
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.

5 participants