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

[REF] duplicate-code: Check state of the lines to strip them #1014

Conversation

moylop260
Copy link
Contributor

@moylop260 moylop260 commented Jul 15, 2016

Fixes / new features

Based on #1012 (comment)

@moylop260 moylop260 force-pushed the master-fix-disable-duplicated-moy2 branch from 24b40a2 to 7c0e8f8 Compare July 15, 2016 16:07
@moylop260 moylop260 force-pushed the master-fix-disable-duplicated-moy2 branch 2 times, most recently from f647b7f to 345e035 Compare July 15, 2016 19:28
@PCManticore
Copy link
Contributor

I'm closing this. I don't like at all that it depends on PyLinter to do its job, it breaks all the principles of encapsulation and data separation. I really think this issue is quite hard to solve now, in a clean way, which is why the issue was added for the 3.0 milestone. There would be a solution, I hope, once we introduce two phases of analysis to Pylint, happening probably in 3.0. This would mean the first phase could collect information about the states in each file, while the second phase can do the disabling and enabling in a consistent way. The only problem with duplicate-code and cyclic-import is that their pragma controls can also depend on their presence in other files, which means, by depending on the state of all the files inside the checkers themselves, we are breaking the encapsulation that checker is running on multiple files or not. The only solution I see right now is the two phase design.

Let's keep it like this for now.

@moylop260
Copy link
Contributor Author

moylop260 commented Jul 17, 2016

The commit
7c0e8f8 is enough to do the work. (Without a pylinter object).

The problem is that current tests don"t use the functional standard tests, use directly a method with sys.argv, then to add the test to check if works I added the pylinter object to make state lines.

Maybe we can live with the fix of the first commit without test for now to avoid add a pylinter... Or migrate a standard functional test instead.

What do you think?

See comment below

@moylop260
Copy link
Contributor Author

moylop260 commented Jul 17, 2016

I have created the correct patch to do its job without use PyLinter

diff --git pylint/checkers/similar.py pylint/checkers/similar.py
index 72358f5..d7fd9db 100644
--- pylint/checkers/similar.py
+++ pylint/checkers/similar.py
@@ -28,7 +28,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
@@ -39,7 +39,7 @@ class Similar(object):
                                          readlines(),
                                          self.ignore_comments,
                                          self.ignore_docstrings,
-                                         self.ignore_imports))
+                                         self.ignore_imports, state_lines))
         except UnicodeDecodeError:
             pass

@@ -123,14 +123,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 \
@@ -155,12 +158,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):
@@ -287,10 +290,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)"""

Do make sense for you this patch without a PyLinter?

@moylop260
Copy link
Contributor Author

@PCManticore
What do you think about my last comment?

@return42
Copy link

@PCManticore this is really a mess. Can you please review @moylop260 patch here in the #1014 (comment) or in the #1055 (comment) / thanks!

return42 added a commit to return42/linuxdoc that referenced this pull request Aug 13, 2018
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.

3 participants