Add flake8 and isort lint checks #46
Conversation
tox.ini
Outdated
|
|
||
| [testenv:lint] | ||
| deps = | ||
| pylint~=2.3.1 |
There was a problem hiding this comment.
Not directly related to this PR, do we want to put a specific version or we want to use the latest version?
Putting a specific version gives us the benefit of a more stable CI enironment (it is not perfect since even if we put a specific package version, it could introduce other package dependencies that are not fixed), in the cost that when we decide to move to a new version, we end up doing a big change (which means bigger risk) instead of doing small changes through our daily work.
There was a problem hiding this comment.
We use ~= (https://www.python.org/dev/peps/pep-0440/#compatible-release) here, but in fact, I think using =2.*,>=2.3.1 or ~=2.3 instead would be better. I would definitely pin the major version though.
There was a problem hiding this comment.
I'd be happy with any of these, from most- to least-conservative: ~=2.3.1, =2.*,>=2.3.1, ~=2.3, >=2.3.1.
Since this is a test dependency and should only affect devs, I think there's actually a good argument for not pinning the major version and staying up to date with the latest versions in the course of regular dev work.
| DUMMY_TRACER = None | ||
| del os.environ[envname] | ||
| self.assertIs(type(tracer), DummyTracer) | ||
| def test_get_envvar_tracer(self): |
There was a problem hiding this comment.
We should be able to capture the missing blank line here?
There was a problem hiding this comment.
Actually these were intentional as the three functions are a "translation" of a single @pytest.mark.parametrize function. But you are right, in general, having a rule to enforce at least a single blank line between functions would be good.
There was a problem hiding this comment.
tox -e lint does catch these, travis just isn't running lint.
| DUMMY_TRACER = None | ||
| del os.environ[envname] | ||
| self.assertIs(type(tracer), DummyTracer) | ||
| def test_get_envvar_tracer(self): |
There was a problem hiding this comment.
Actually these were intentional as the three functions are a "translation" of a single @pytest.mark.parametrize function. But you are right, in general, having a rule to enforce at least a single blank line between functions would be good.
tox.ini
Outdated
|
|
||
| [testenv:lint] | ||
| deps = | ||
| pylint~=2.3.1 |
There was a problem hiding this comment.
We use ~= (https://www.python.org/dev/peps/pep-0440/#compatible-release) here, but in fact, I think using =2.*,>=2.3.1 or ~=2.3 instead would be better. I would definitely pin the major version though.
|
Lint failures right on cue:
|
|
I see. Why did you move the linter out from the default testenv section? |
I thought it made it easier to read now that there are multiple lint commands, but I don't have any strong preferences about how the toxfile is organized. |
|
Hopefully using checkfiles=$(comm -12 <(git diff --name-only --cached HEAD | sort) <(find opentelemetry-*/src -type f -name "*.py" | sort))
[[ $checkfiles ]] && isort $checkfiles |
* Add context-base and context-asynchooks packages * add BaseScopeManager interface open-telemetry#46
From #42 (comment), builds on #42. Compare to @Oberon00's
unit-testsbranch for a sane diff: Oberon00/opentelemetry-python@unit-tests...c24t:more-pedantic-lintThis PR makes lint a bit stricter by adding
flake8andisortchecks.