-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[FIX] duplicate-code: ignored disable=duplicate-code comments #1055
[FIX] duplicate-code: ignored disable=duplicate-code comments #1055
Conversation
e30faca
to
0f1f14f
Compare
0f1f14f
to
6a4e8db
Compare
@PCManticore |
Any chance this is being merged? @moylop260 Can you resolve the conflict with |
6a4e8db
to
7862b58
Compare
Fixed conflicts |
@moylop260 There are new conflicts in this PR. Regards |
7862b58
to
679f934
Compare
@hbto I let the message and the change deleted here:
|
[REF] travis_install_nightly: Add ugly patch from pylint-dev/pylint#1055
Is there anything holding this back from getting merged? Anything that can be fixed? |
Currently I'm applying this patch from our CI system using the following lines: wget -q https://patch-diff.githubusercontent.com/raw/PyCQA/pylint/pull/1055.patch -O /tmp/pylint_pr1055.patch
patch -f -p0 $(python -c "from pylint.checkers import similar;print similar.__file__.rstrip('c')") -i /tmp/pylint_pr1055.patch You can use them since that this fix currently is not priority for maintainers |
I am all for merging it, but we need automated tests that confirms new feature works. |
679f934
to
5e3ceb5
Compare
@rogalski The tests of the duplicated code are executed from a |
@moylop260 can we please keep pytest-style functional tests instead of introducing a class? Other than that, it looks great. |
30ac482
to
5bcf44e
Compare
- [IMP] duplicate-code: Add support to get state lines
@rogalski Done! Using pytest-style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it's ready to be merged.
The only thing to be discussed is creating this fake PyLinter instance just to get state lines. Maybe we can extract this functionality to more manageable form.
@@ -327,6 +336,27 @@ def usage(status=0): | |||
[-i|--ignore-comments] [--ignore-docstrings] [--ignore-imports] file1...') | |||
sys.exit(status) | |||
|
|||
|
|||
def _get_state_lines(linter, filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This almost look like we need to extract this functionality from PyLinter class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of understand it
Do you have a example to clarify it?
I only mean that information about locally disabled messages should be acquirable without creating an instance of PyLinter. I'm not sure if this is possible right now, I'm not familiar with this part of repository. I'll check it and get back to you.
Dnia 14.08.2017 o godz. 23:15 moylop260 <[email protected]> napisał(a):
… @moylop260 commented on this pull request.
In pylint/checkers/similar.py:
> @@ -327,6 +336,27 @@ def usage(status=0):
[-i|--ignore-comments] [--ignore-docstrings] [--ignore-imports] file1...')
sys.exit(status)
+
+def _get_state_lines(linter, filename):
I'm not sure of understand it
Do you have a example to clarify it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Folks, as I already mentioned in one of the many iterations of this PR (#1014), I'm not going to merge this PR in this state, where the file state is retrieved as is by instantiating a PyLinter class and whatnot. One has to to come up with a better engineered solution for solving this problem, so let's stop for a moment, let's identify how we can approach this in a sane manner and after that we can write the code. |
The pylinter is not required to do the work. We are using this patch 1 year ago working without problem. Could you help us to identify why the first commit/soluction is a bad engineered, please? |
I guess this one rotted away? It would be swell to be able to disable duplicate-code for one particular file in my project. I'm probably going to skip all linting of that file to get that effect which is much less desirable. Seems like the patch here avoids the criticisms elsewhere of dependence on a PyLinter instance and not having any tests and could be merged. But I'm no expert on pylint :D Was another solution ever found? |
Yes, you are right. I was banned to contribute with this project. But you can use the following patch in order to fix it locally or for your CI using the following command: wget -q https://raw.githubusercontent.com/Vauxoo/pylint-conf/master/conf/pylint_pr1055.patch -O /tmp/pylint_pr1055.patch
patch -f -p0 $(python -c "from __future__ import print_function; from pylint.checkers import similar; print(similar.__file__.rstrip('c'))") -i /tmp/pylint_pr1055.patch UPDATED 2019-03-07: |
I mean, the minimal patch to fix this too old issue is (more than 4 years ago): diff --git a/pylint/checkers/similar.py b/pylint/checkers/similar.py
index aa59bfd..a7c0c0e 100644
--- a/pylint/checkers/similar.py
+++ b/pylint/checkers/similar.py
@@ -31,7 +31,7 @@ class Similar(object):
self.ignore_imports = ignore_imports
self.linesets = []
- def append_stream(self, streamid, stream, encoding=None):
+ def append_stream(self, streamid, stream, encoding=None, state_lines=None):
"""append a file to search for similarities"""
if encoding is None:
readlines = stream.readlines
@@ -42,7 +42,7 @@ class Similar(object):
readlines(),
self.ignore_comments,
self.ignore_docstrings,
- self.ignore_imports))
+ self.ignore_imports, state_lines))
except UnicodeDecodeError:
pass
@@ -126,14 +126,17 @@ class Similar(object):
for sim in self._find_common(lineset, lineset2):
yield sim
-def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports):
+def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports, state_lines=None):
"""return lines with leading/trailing whitespace and any ignored code
features removed
"""
strippedlines = []
docstring = None
- for line in lines:
+ for lineno, line in enumerate(lines, start=1):
+ if state_lines and lineno in state_lines and not state_lines[lineno]:
+ strippedlines.append('')
+ continue
line = line.strip()
if ignore_docstrings:
if not docstring and \
@@ -158,12 +161,12 @@ def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports):
class LineSet(object):
"""Holds and indexes all the lines of a single source file"""
def __init__(self, name, lines, ignore_comments=False,
- ignore_docstrings=False, ignore_imports=False):
+ ignore_docstrings=False, ignore_imports=False, state_lines=None):
self.name = name
self._real_lines = lines
self._stripped_lines = stripped_lines(lines, ignore_comments,
ignore_docstrings,
- ignore_imports)
+ ignore_imports, state_lines)
self._index = self._mk_index()
def __str__(self):
@@ -290,10 +293,11 @@ class SimilarChecker(BaseChecker, Similar):
stream must implement the readlines method
"""
+ state_lines = self.linter.file_state._module_msgs_state.get('R0801')
with node.stream() as stream:
self.append_stream(self.linter.current_name,
stream,
- node.file_encoding)
+ node.file_encoding, state_lines)
def close(self):
"""compute and display similarities on closing (i.e. end of parsing)"""
|
@moylop260 Your fix doesn't work. Even if I have |
@filips123 |
Here are versions:
And some of my code which is duplicated is: @staticmethod
def __zip_directory(source, destination):
"""Make ZIP archive from directory."""
zipf = zipfile.ZipFile(destination, 'w', zipfile.ZIP_DEFLATED)
for root, _, files in os.walk(source):
for file in files:
real = os.path.join(root, file)
name = real[len(source):]
zipf.write(real, name)
zipf.close()
@staticmethod
def __unzip_directory(source, destination):
"""Extract ZIP archive to directory."""
zipf = zipfile.ZipFile(source)
zipf.extractall(destination)
zipf.close() I can't use the shared code for this because there are some changes in every function. I fixed this for now with specifying high |
@filips123 If yes, then you need apply it manually since that it patch was created 3 years ago and it could have |
Yes, there are errors:
And --- a/pylint/checkers/similar.py
+++ b/pylint/checkers/similar.py
@@ -31,7 +31,7 @@ class Similar(object):
self.ignore_imports = ignore_imports
self.linesets = []
- def append_stream(self, streamid, stream, encoding=None):
+ def append_stream(self, streamid, stream, encoding=None, state_lines=None):
"""append a file to search for similarities"""
if encoding is None:
readlines = stream.readlines
@@ -42,7 +42,7 @@ class Similar(object):
readlines(),
self.ignore_comments,
self.ignore_docstrings,
- self.ignore_imports))
+ self.ignore_imports, state_lines))
except UnicodeDecodeError:
pass
@@ -126,14 +126,17 @@ class Similar(object):
for sim in self._find_common(lineset, lineset2):
yield sim
-def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports):
+def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports, state_lines=None):
"""return lines with leading/trailing whitespace and any ignored code
features removed
"""
strippedlines = []
docstring = None
- for line in lines:
+ for lineno, line in enumerate(lines, start=1):
+ if state_lines and lineno in state_lines and not state_lines[lineno]:
+ strippedlines.append('')
+ continue
line = line.strip()
if ignore_docstrings:
if not docstring and \
@@ -158,12 +161,12 @@ def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports):
class LineSet(object):
"""Holds and indexes all the lines of a single source file"""
def __init__(self, name, lines, ignore_comments=False,
- ignore_docstrings=False, ignore_imports=False):
+ ignore_docstrings=False, ignore_imports=False, state_lines=None):
self.name = name
self._real_lines = lines
self._stripped_lines = stripped_lines(lines, ignore_comments,
ignore_docstrings,
- ignore_imports)
+ ignore_imports, state_lines)
self._index = self._mk_index()
def __str__(self):
@@ -290,10 +293,11 @@ class SimilarChecker(BaseChecker, Similar):
stream must implement the readlines method
"""
+ state_lines = self.linter.file_state._module_msgs_state.get('R0801')
with node.stream() as stream:
self.append_stream(self.linter.current_name,
stream,
- node.file_encoding)
+ node.file_encoding, state_lines)
def close(self):
"""compute and display similarities on closing (i.e. end of parsing)""" |
For pylint >= 2.2.0 The new patch is: wget -q https://raw.githubusercontent.com/Vauxoo/pylint-conf/master/conf/pylint_pr1055_2.patch -O /tmp/pylint_pr1055_2.patch
patch -f -p0 $(python -c "from __future__ import print_function; from pylint.checkers import similar; print(similar.__file__.rstrip('c'))") -i /tmp/pylint_pr1055_2.patch For older ones use: wget -q https://raw.githubusercontent.com/Vauxoo/pylint-conf/master/conf/pylint_pr1055.patch -O /tmp/pylint_pr1055.patch
patch -f -p0 $(python -c "from __future__ import print_function; from pylint.checkers import similar; print(similar.__file__.rstrip('c'))") -i /tmp/pylint_pr1055.patch |
We're hitting this issue with AWS Lambda functions. Nothing shared so duplication is expected. What is the ETA on this being in a release rather than patch? |
Hi @mattcanty I'm not a maintainer of this project then I can't tell you a ETA to release a fix. You can disable this check or increase the min-similarity-lines parameter. |
Here is a way to get around this issue : edit your pylintrc file and add: # Minimum lines number of a similarity.
min-similarity-lines=100 Setting a high value to |
…sion This patch is to fix an issue about pylint disable in duplicate-code, related to PR pylint-dev/pylint#1055, this patch works in pylint versions >= 2.2.0
…sion This patch is to fix an issue about pylint disable in duplicate-code, related to PR pylint-dev/pylint#1055, this patch works in pylint versions >= 2.2.0
…sion This patch is to fix an issue about pylint disable in duplicate-code, related to PR pylint-dev/pylint#1055, this patch works in pylint versions >= 2.2.0
…sion This patch is to fix an issue about pylint disable in duplicate-code, related to PR pylint-dev/pylint#1055, this patch works in pylint versions >= 2.2.0
…sion (#318) This patch is to fix an issue about pylint disable in duplicate-code, related to PR pylint-dev/pylint#1055, this patch works in pylint versions >= 2.2.0
…sion This patch is to fix an issue about pylint disable in duplicate-code, related to PR pylint-dev/pylint#1055, this patch works in pylint versions >= 2.2.0
Fixes / new features
It's a proposal from last comment #1014 (comment) from 15 days ago.
Note:
Just I could add a reference in the original issue to other people that want apply this patch.