pylint: enable additional checks #4122
Conversation
skshetry
force-pushed
the
pylint-additional-checks
branch
2 times, most recently
from
85622a4 to
3001392
Compare
Codecov Report
@@ Coverage Diff @@
## master #4122 +/- ##
==========================================
- Coverage 92.76% 92.70% -0.07%
==========================================
Files 162 162
Lines 11278 11288 +10
==========================================
+ Hits 10462 10464 +2
- Misses 816 824 +8
Continue to review full report at Codecov.
|
Created a separate pylint plugin to disable some checks for tests.
skshetry
force-pushed
the
pylint-additional-checks
branch
from
3001392 to
3259a43
Compare
| # Enabling soon: | ||
| no-member, | ||
| unused-argument, | ||
| arguments-differ, |
There was a problem hiding this comment.
Only checks that are important is for missing docstring and logging format (with lazy logging).
| except Exception as exc: # pylint: disable=broad-except | ||
| if isinstance(exc, OSError) and exc.errno == errno.EMFILE: | ||
| except OSError as exc: | ||
| if exc.errno == errno.EMFILE: |
There was a problem hiding this comment.
Changed code to make pylint understand Exception better.
There was a problem hiding this comment.
This one is questionable, just duplicating the code :(
There was a problem hiding this comment.
I think it's worth it to separate exception here. The duplication is 2 lines and most of it is incidental, the return code and message can change independently like in the above try-except ladder.
I could do:
if ret == 255:
logger.info("unexpected error")But, I don't think it's worth the hassle.
| @@ -100,7 +100,7 @@ class BaseRemoteTree: | |||
|
|
|||
| CACHE_MODE = None | |||
| SHARED_MODE_MAP = {None: (None, None), "group": (None, None)} | |||
| CHECKSUM_DIR_SUFFIX = ".dir" | |||
| @@ -79,7 +79,7 @@ def getsize(self, path): | |||
|
|
|||
| return 0 | |||
|
|
|||
| def exists(self, path, sftp=None): | |||
| @@ -841,7 +841,7 @@ def test(self): | |||
| self.assertTrue(filecmp.cmp(foo, bar, shallow=False)) | |||
|
|
|||
|
|
|||
| class TestReproExternalBase(SingleStageRun, TestDvc): | |||
| class ReproExternalTestMixin(SingleStageRun, TestDvc): | |||
There was a problem hiding this comment.
pylint understands use of Mixin, so, it does not throw any error if any member is not found. eg: self.bucket.
| @@ -0,0 +1,47 @@ | |||
| """ | |||
There was a problem hiding this comment.
Pylint does not allow different configs per directory. It requires a "run-twice" or a "disable-on-each-file" solution.
Therefore, I wrote this hack/plugin for now. If it becomes hard to maintain, I'll just remove this and substitute with "run-twice" solution with two .pylintrc, one for dvc and one for tests.
skshetry
changed the title
| init-hook="import sys; \ | ||
| from pathlib import Path; \ | ||
| from pylint.config import find_pylintrc; \ | ||
| sys.path.append(Path(find_pylintrc()).parent / 'tests');" | ||
|
|
There was a problem hiding this comment.
Required for loading the plugin.
| @@ -413,7 +419,7 @@ def upload(self, from_info, to_info, name=None, no_progress_bar=False): | |||
|
|
|||
| name = name or from_info.name | |||
|
|
|||
| self._upload( | |||
| self._upload( # noqa, pylint: disable=no-member | |||
There was a problem hiding this comment.
I think, we should create an _upload/_download function in the base class that just throws RemoteActionNotImplemented error. It would make this cleaner (without ignore, that is), and provide stronger checks-and-balances.
|
|
||
| def is_node_in_tests(node: "node_classes.NodeNG"): | ||
| module: "scoped_nodes.Module" = node.root() | ||
| return module.file.startswith(TESTS_FOLDER) |
| def open(self, path, mode="r", encoding="utf-8", remote=None): | ||
| def open( | ||
| self, path, mode="r", encoding="utf-8", remote=None | ||
| ): # pylint: disable=arguments-differ |
There was a problem hiding this comment.
Should we start preferring the use of **kwargs in these kinds of cases instead of suppressing this pylint test each time?
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
β I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. π