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 disabled from first line to top code line #1012

Conversation

moylop260
Copy link
Contributor

@moylop260 moylop260 commented Jul 14, 2016

Fixes / new features

@moylop260 moylop260 changed the title [REF] similar: If is not enabled then is not checked [REF] duplicate-code: If is not enabled then is not checked Jul 14, 2016
@moylop260 moylop260 force-pushed the master-fix-disable-duplicated-moy branch from 92ecf56 to 7622b2a Compare July 15, 2016 00:12
@moylop260 moylop260 changed the title [REF] duplicate-code: If is not enabled then is not checked [REF] duplicate-code: Check disabled from first line to top code line Jul 15, 2016
@moylop260
Copy link
Contributor Author

I have 2 proposal to fix it but I don't know what is better for you.

  • Option to change the core:

    diff --git a/pylint/utils.py b/pylint/utils.py
    index 53d31f3..f2b2c6a 100644
    --- a/pylint/utils.py
    +++ b/pylint/utils.py
    @@ -576,6 +576,19 @@ class FileState(object):
         for msgid, lines in six.iteritems(msg_state):
             for lineno, state in list(lines.items()):
                 original_lineno = lineno
    +                if lineno < firstchildlineno and isinstance(node, nodes.Module):
    +                    # *disable*/*enable* comment is on top then apply to previous lines
    +                    # 1.     # comment 1
    +                    # 2.     # comment 2
    +                    # 3.     # pylint: disable=E1101
    +                    # 4.     var = True
    +                    # E1101 should be disable from line 1 to 4, because is on top.
    +                    for lineno_before_firstchildlineno in range(0, firstchildlineno):
    +                        # Include zero because astroid.Module use the zero as first node.lineno
    +                        try:
    +                            self._module_msgs_state[msgid][lineno_before_firstchildlineno] = state
    +                        except KeyError:
    +                            self._module_msgs_state[msgid] = {lineno_before_firstchildlineno: state}
                 if first > lineno or last < lineno:
                     continue
                 # Set state for all lines for this block, if the
    diff --git a/pylint/checkers/similar.py b/pylint/checkers/similar.py
    index 72358f5..4bb3bf5 100644
    --- a/pylint/checkers/similar.py
    +++ b/pylint/checkers/similar.py
    @@ -287,6 +287,9 @@ class SimilarChecker(BaseChecker, Similar):
    
         stream must implement the readlines method
         """
    +        if not self.linter.is_message_enabled('R0801', node.lineno):
    +            return
    +
         with node.stream() as stream:
             self.append_stream(self.linter.current_name,
                                stream,
  • Option to change just similar.py:

    diff --git a/pylint/checkers/similar.py b/pylint/checkers/similar.py
    index 72358f5..caad71a 100644
    --- a/pylint/checkers/similar.py
    +++ b/pylint/checkers/similar.py
    @@ -287,6 +287,12 @@ class SimilarChecker(BaseChecker, Similar):
    
         stream must implement the readlines method
         """
    +        if not self.linter.is_message_enabled('R0801') or not node.body:
    +            return
    +
    +        for top_line in range(1, node.body[0].lineno):
    +            if not self.linter.is_message_enabled('R0801', top_line):
    +                return
         with node.stream() as stream:
             self.append_stream(self.linter.current_name,
                                stream,
  • Other one?

@PCManticore
Copy link
Contributor

I am going to close this, it is too hackish to even take in consideration. As seen from the issue you've linked, the semantics should be as close as possible to those of the current pragma handling, which means disabling for some blocks of duplicate code in a file should not disable for all the duplicate blocks in the same file. Specializing the behavior to stop counting as soon as we encounter a disabled message is not good, since afterwards, the user cannot enable the message again in the same file. If you really want to tackle this come with a better plan of how to handle the same semantics for all the files, with semantics for separate blocks from each file.

For instance, if I have file a.py as:

...
...
# disable=duplicate-code; don't want for this block
def test(...):
    pass

# enable=duplicate-code; want for all the blocks from here

and a file b.py without any disable for duplicate-code, I would still want the duplicate blocks to be found in b.py, as well between b.py and a.py, excepting the ones which were scoped out by the pragma.
I think this makes more sense and would be more predictible than this patch, but at the same time, it is a more challenging implementation.

@moylop260
Copy link
Contributor Author

moylop260 commented Jul 15, 2016

Ok, I got it
I'll propagate the state_lines = self.linter.file_state._module_msgs_state.get('R0801') to others methods to process it from similar.py

Thanks for feedback

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.

2 participants