Skip to content

pylint: enable additional checks #4122

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

Merged
merged 1 commit into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ ignore-patterns=

# Python code to execute, usually for sys.path manipulation such as
# pygtk.require().
#init-hook=
init-hook="import sys; \
from pathlib import Path; \
from pylint.config import find_pylintrc; \
sys.path.append(Path(find_pylintrc()).parent / 'tests');"

Comment on lines +18 to +22
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Required for loading the plugin.


# Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the
# number of processors available to use.
Expand All @@ -28,7 +32,7 @@ limit-inference-results=100

# List of plugins (as comma separated values of python modules names) to load,
# usually to register additional checkers.
load-plugins=pylint_pytest
load-plugins=pylint_pytest,pylint_plugin_disable

# Pickle collected data for later comparisons.
persistent=yes
Expand Down Expand Up @@ -149,10 +153,8 @@ disable=format,
exception-escape,
comprehension-escape,
# custom disables start here
protected-access, # needs-refactor
no-self-use,
invalid-name,
blacklisted-name,
# already a check enabled for `wildcard-import`
unused-wildcard-import,
misplaced-comparison-constant,
Expand All @@ -176,10 +178,6 @@ disable=format,
# we use it to speedup/optimize
import-outside-toplevel,
fixme,
# Enabling soon:
no-member,
unused-argument,
arguments-differ,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only checks that are important is for missing docstring and logging format (with lazy logging).



# Enable the message, report, category or checker with the given id(s). You can
Expand Down Expand Up @@ -267,7 +265,7 @@ contextmanager-decorators=contextlib.contextmanager
# List of members which are set dynamically and missed by pylint inference
# system, and so shouldn't trigger E1101 when accessed. Python regular
# expressions are accepted.
generated-members=
generated-members=pytest.lazy_fixture,logging.TRACE,logger.trace,sys.getwindowsversion

# Tells whether missing members accessed in mixin class should be ignored. A
# mixin class is detected if its name ends with "mixin" (case insensitive).
Expand All @@ -288,7 +286,7 @@ ignore-on-opaque-inference=yes
# List of class names for which member attributes should not be checked (useful
# for classes with dynamically set attributes). This supports the use of
# qualified names.
ignored-classes=optparse.Values,thread._local,_thread._local
ignored-classes=optparse.Values,thread._local,_thread._local,Dvcfile

# List of module names for which member attributes should not be checked
# (useful for modules/projects where namespaces are manipulated during runtime
Expand Down Expand Up @@ -329,7 +327,7 @@ dummy-variables-rgx=_+$|(_[a-zA-Z0-9_]*[a-zA-Z0-9]+?$)|dummy|^ignored_|^unused_

# Argument names that match this expression will be ignored. Default to name
# with leading underscore.
ignored-argument-names=_.*|^ignored_|^unused_
ignored-argument-names=_.*|^ignored_|^unused_|args|kwargs

# Tells whether we should check for unused import in __init__ files.
init-import=no
Expand Down
2 changes: 1 addition & 1 deletion dvc/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def get_url(path, repo=None, rev=None, remote=None):
"""
with _make_repo(repo, rev=rev) as _repo:
if not isinstance(_repo, Repo):
raise UrlNotDvcRepoError(_repo.url)
raise UrlNotDvcRepoError(_repo.url) # pylint: disable=no-member
out = _repo.find_out_by_relpath(path)
remote_obj = _repo.cloud.get_remote(remote)
return str(remote_obj.hash_to_path_info(out.checksum))
Expand Down
1 change: 1 addition & 0 deletions dvc/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ def update(self, item, suffix=""):


class NamedCache:
# pylint: disable=protected-access
def __init__(self):
self._items = defaultdict(lambda: defaultdict(NamedCacheItem))
self.external = defaultdict(set)
Expand Down
2 changes: 1 addition & 1 deletion dvc/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
class DvcParser(argparse.ArgumentParser):
"""Custom parser class for dvc CLI."""

def error(self, message, command=None):
def error(self, message, command=None): # pylint: disable=arguments-differ
"""Custom error method.
Args:
message (str): error message.
Expand Down
3 changes: 3 additions & 0 deletions dvc/command/git_hook.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ def run(self):

return self._run()

def _run(self):
raise NotImplementedError


class CmdPreCommit(CmdHookBase):
def _run(self):
Expand Down
8 changes: 5 additions & 3 deletions dvc/dvcfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def validate(cls, d, fname=None):
except MultipleInvalid as exc:
raise StageFileFormatError(f"'{fname}' format error: {exc}")

def remove(self, force=False):
def remove(self, force=False): # pylint: disable=unused-argument
with contextlib.suppress(FileNotFoundError):
os.unlink(self.path)

Expand Down Expand Up @@ -141,7 +141,7 @@ def dump(self, stage, **kwargs):
dump_yaml(self.path, serialize.to_single_stage_file(stage))
self.repo.scm.track_file(self.relpath)

def remove_stage(self, stage):
def remove_stage(self, stage): # pylint: disable=unused-argument
self.remove()


Expand All @@ -154,7 +154,9 @@ class PipelineFile(FileMixin):
def _lockfile(self):
return Lockfile(self.repo, os.path.splitext(self.path)[0] + ".lock")

def dump(self, stage, update_pipeline=False, no_lock=False):
def dump(
self, stage, update_pipeline=False, no_lock=False, **kwargs
): # pylint: disable=arguments-differ
"""Dumps given stage appropriately in the dvcfile."""
from dvc.stage import PipelineStage

Expand Down
1 change: 1 addition & 0 deletions dvc/external_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ def clean_repos():


class BaseExternalRepo:
# pylint: disable=no-member

_local_cache = None

Expand Down
2 changes: 1 addition & 1 deletion dvc/lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ def __init__(
def lockfile(self):
return self._lockfile

def lock(self):
def lock(self): # pylint: disable=arguments-differ
try:
super().lock(timedelta(seconds=DEFAULT_TIMEOUT))
except flufl.lock.TimeOutError:
Expand Down
1 change: 1 addition & 0 deletions dvc/logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ def addLoggingLevel(levelName, levelNum, methodName=None):

def logForLevel(self, message, *args, **kwargs):
if self.isEnabledFor(levelNum):
# pylint: disable=protected-access
self._log(levelNum, message, args, **kwargs)

def logToRoot(message, *args, **kwargs):
Expand Down
9 changes: 6 additions & 3 deletions dvc/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ def main(argv=None):
except DvcException:
ret = 255
logger.exception("")
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:
Comment on lines -68 to +69
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed code to make pylint understand Exception better.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one is questionable, just duplicating the code :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

logger.exception(
"too many open files, please visit "
"{} to see how to handle this "
Expand All @@ -78,10 +78,13 @@ def main(argv=None):
else:
logger.exception("unexpected error")
ret = 255
except Exception: # noqa, pylint: disable=broad-except
logger.exception("unexpected error")
ret = 255
finally:
logger.setLevel(outerLogLevel)

# Closing pools by-hand to prevent wierd messages when closing SSH
# Closing pools by-hand to prevent weird messages when closing SSH
# connections. See https://github.com/iterative/dvc/issues/3248 for
# more info.
close_pools()
Expand Down
5 changes: 5 additions & 0 deletions dvc/output/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@ def isdir(self):
def isfile(self):
return self.remote.tree.isfile(self.path_info)

# pylint: disable=no-member

def ignore(self):
if not self.use_scm_ignore:
return
Expand All @@ -245,6 +247,8 @@ def ignore_remove(self):

self.repo.scm.ignore_remove(self.fspath)

# pylint: enable=no-member

def save(self):
if not self.exists:
raise self.DoesNotExistError(self)
Expand Down Expand Up @@ -346,6 +350,7 @@ def remove(self, ignore_remove=False):
self.ignore_remove()

def move(self, out):
# pylint: disable=no-member
if self.scheme == "local" and self.use_scm_ignore:
self.repo.scm.ignore_remove(self.fspath)

Expand Down
7 changes: 4 additions & 3 deletions dvc/path_info.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# pylint: disable=protected-access
import os
import pathlib
import posixpath
Expand All @@ -17,7 +18,7 @@ def overlaps(self, other):
return self.isin_or_eq(other) or other.isin(self)

def isin_or_eq(self, other):
return self == other or self.isin(other)
return self == other or self.isin(other) # pylint: disable=no-member


class PathInfo(pathlib.PurePath, _BasePath):
Expand All @@ -37,7 +38,7 @@ def __new__(cls, *args):
return cls._from_parts(args)

def as_posix(self):
f = self._flavour
f = self._flavour # pylint: disable=no-member
# Unlike original implementation [1] that uses `str()` we actually need
# to use `fspath`, because we've overridden `__str__` method to return
# relative paths, which will break original `as_posix`.
Expand Down Expand Up @@ -264,7 +265,7 @@ def from_parts(
params=None,
query=None,
fragment=None,
):
): # pylint: disable=arguments-differ
assert bool(host) ^ bool(netloc)

if netloc is not None:
Expand Down
15 changes: 11 additions & 4 deletions dvc/remote/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class BaseRemoteTree:

CACHE_MODE = None
SHARED_MODE_MAP = {None: (None, None), "group": (None, None)}
CHECKSUM_DIR_SUFFIX = ".dir"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duplicated above

PARAM_CHECKSUM = None

state = StateNoop()

Expand All @@ -119,6 +119,7 @@ def __init__(self, repo, config):
or self.HASH_JOBS
)
self.verify = config.get("verify", self.DEFAULT_VERIFY)
self.path_info = None

@classmethod
def get_missing_deps(cls):
Expand Down Expand Up @@ -184,14 +185,17 @@ def cache(self):

def open(self, path_info, mode="r", encoding=None):
if hasattr(self, "_generate_download_url"):
get_url = partial(self._generate_download_url, path_info)
func = self._generate_download_url # noqa,pylint:disable=no-member
get_url = partial(func, path_info)
return open_url(get_url, mode=mode, encoding=encoding)

raise RemoteActionNotImplemented("open", self.scheme)

def exists(self, path_info):
raise NotImplementedError

# pylint: disable=unused-argument

def isdir(self, path_info):
"""Optional: Overwrite only if the remote has a way to distinguish
between a directory and a file.
Expand Down Expand Up @@ -250,6 +254,8 @@ def protect(path_info):
def is_protected(self, path_info):
return False

# pylint: enable=unused-argument

@staticmethod
def unprotect(path_info):
pass
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

from_info.fspath,
to_info,
name=name,
Expand Down Expand Up @@ -504,7 +510,7 @@ def _download_file(

tmp_file = tmp_fname(to_info)

self._download(
self._download( # noqa, pylint: disable=no-member
from_info, tmp_file, name=name, no_progress_bar=no_progress_bar
)

Expand Down Expand Up @@ -866,6 +872,7 @@ def gc(cls, named_cache, remote, jobs=None):
path_info = tree.hash_to_path_info(hash_)
if tree.is_dir_hash(hash_):
# backward compatibility
# pylint: disable=protected-access
tree._remove_unpacked_dir(hash_)
tree.remove(path_info)
removed = True
Expand Down
4 changes: 3 additions & 1 deletion dvc/remote/gs.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def wrapper(*args, **kwargs):
while True:
try:
# skipcq: PYL-W0212
# pylint: disable=protected-access
chunk_size = Blob._CHUNK_SIZE_MULTIPLE * multiplier
return func(*args, chunk_size=chunk_size, **kwargs)
except requests.exceptions.ConnectionError:
Expand Down Expand Up @@ -102,7 +103,8 @@ def _generate_download_url(self, path_info, expires=3600):
raise FileNotFoundError

if isinstance(
blob.client._credentials, google.auth.credentials.Signing
blob.client._credentials, # pylint: disable=protected-access
google.auth.credentials.Signing,
):
# sign if we're able to sign with credentials.
return blob.generate_signed_url(expiration=expiration)
Expand Down
7 changes: 5 additions & 2 deletions dvc/remote/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ def wrapper(from_info, to_info, *args, **kwargs):
except Exception as exc: # pylint: disable=broad-except
# NOTE: this means we ran out of file descriptors and there is no
# reason to try to proceed, as we will hit this error anyways.
# pylint: disable=no-member
if isinstance(exc, OSError) and exc.errno == errno.EMFILE:
raise

Expand Down Expand Up @@ -362,7 +363,7 @@ def cache_dir(self, value):
self.tree.path_info = PathInfo(value) if value else None

@classmethod
def supported(cls, config):
def supported(cls, config): # pylint: disable=unused-argument
return True

@cached_property
Expand All @@ -375,7 +376,9 @@ def hash_to_path(self, hash_):
# being ~5.5 times faster.
return f"{self.cache_path}{os.sep}{hash_[0:2]}{os.sep}{hash_[2:]}"

def hashes_exist(self, hashes, jobs=None, name=None):
def hashes_exist(
self, hashes, jobs=None, name=None
): # pylint: disable=unused-argument
return [
hash_
for hash_ in Tqdm(
Expand Down
2 changes: 1 addition & 1 deletion dvc/remote/ssh/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def getsize(self, path):

return 0

def exists(self, path, sftp=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unused arg.

def exists(self, path):
return bool(self.st_mode(path))

def isdir(self, path):
Expand Down
1 change: 1 addition & 0 deletions dvc/repo/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def locked(f):
@wraps(f)
def wrapper(repo, *args, **kwargs):
with repo.lock, repo.state:
# pylint: disable=protected-access
repo._reset()
ret = f(repo, *args, **kwargs)
# Our graph cache is no longer valid after we release the repo.lock
Expand Down
2 changes: 1 addition & 1 deletion dvc/repo/add.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def _create_stages(repo, targets, fname, pbar=None, external=False):
if stage:
Dvcfile(repo, stage.path).remove()

repo._reset()
repo._reset() # pylint: disable=protected-access

if not stage:
if pbar is not None:
Expand Down
Loading