Merge branch 'bug/257' into 'master'

Fix subtle reporting bug for default on plugins

When we refactored our decision process to fix #239, we broke a subtle
corner case where extensions that are not off-by-default are to be
reported.

This further refactors that logic and adds specific tests around it to
ensure it works as expected and doesn't regress.

Closes #257

See merge request !148

5 files changed, 116 insertions, 27 deletions

diff --git a/docs/source/release-notes/3.2.1.rst b/docs/source/release-notes/3.2.1.rst
index dead1db..5fd3375 100644
--- a/docs/source/release-notes/3.2.1.rst
+++ b/docs/source/release-notes/3.2.1.rst
@@ -3,6 +3,11 @@
 
 You can view the `3.2.1 milestone`_ on GitLab for more details.
 
+- Fix subtle bug when deciding whether to report an on-by-default's violation
+  (See also `GitLab#257`_)
+
 .. links
 .. _3.2.1 milestone:
     https://gitlab.com/pycqa/flake8/milestones/15
+.. _GitLab#257:
+    https://gitlab.com/pycqa/flake8/issues/257
diff --git a/src/flake8/defaults.py b/src/flake8/defaults.py
index e3ac9fb..340e8a9 100644
--- a/src/flake8/defaults.py
+++ b/src/flake8/defaults.py
@@ -1,9 +1,28 @@
 """Constants that define defaults."""
 import re
 
-EXCLUDE = '.svn,CVS,.bzr,.hg,.git,__pycache__,.tox,.eggs,*.egg'
-IGNORE = 'E121,E123,E126,E226,E24,E704,W503,W504'
-SELECT = 'E,F,W,C90'
+EXCLUDE = (
+    '.svn',
+    'CVS',
+    '.bzr',
+    '.hg',
+    '.git',
+    '__pycache__',
+    '.tox',
+    '.eggs',
+    '*.egg',
+)
+IGNORE = (
+    'E121',
+    'E123',
+    'E126',
+    'E226',
+    'E24',
+    'E704',
+    'W503',
+    'W504',
+)
+SELECT = ('E', 'F', 'W', 'C90')
 MAX_LINE_LENGTH = 79
 
 TRUTHY_VALUES = set(['true', '1', 't'])
diff --git a/src/flake8/main/options.py b/src/flake8/main/options.py
index 7e9b79e..9780378 100644
--- a/src/flake8/main/options.py
+++ b/src/flake8/main/options.py
@@ -63,7 +63,7 @@ def register_default_options(option_manager):
     )
 
     add_option(
-        '--exclude', metavar='patterns', default=defaults.EXCLUDE,
+        '--exclude', metavar='patterns', default=','.join(defaults.EXCLUDE),
         comma_separated_list=True, parse_from_config=True,
         normalize_paths=True,
         help='Comma-separated list of files or directories to exclude.'
@@ -102,7 +102,7 @@ def register_default_options(option_manager):
     )
 
     add_option(
-        '--ignore', metavar='errors', default=defaults.IGNORE,
+        '--ignore', metavar='errors', default=','.join(defaults.IGNORE),
         parse_from_config=True, comma_separated_list=True,
         help='Comma-separated list of errors and warnings to ignore (or skip).'
              ' For example, ``--ignore=E4,E51,W234``. (Default: %default)',
@@ -116,7 +116,7 @@ def register_default_options(option_manager):
     )
 
     add_option(
-        '--select', metavar='errors', default=defaults.SELECT,
+        '--select', metavar='errors', default=','.join(defaults.SELECT),
         parse_from_config=True, comma_separated_list=True,
         help='Comma-separated list of errors and warnings to enable.'
              ' For example, ``--select=E4,E51,W234``. (Default: %default)',
diff --git a/src/flake8/style_guide.py b/src/flake8/style_guide.py
index 9f3b86f..a531b39 100644
--- a/src/flake8/style_guide.py
+++ b/src/flake8/style_guide.py
@@ -63,10 +63,16 @@ class StyleGuide(object):
         self.formatter = formatter
         self.stats = statistics.Statistics()
         self._selected = tuple(options.select)
-        self._extended_selected = tuple(options.extended_default_select)
+        self._extended_selected = tuple(sorted(
+            options.extended_default_select,
+            reverse=True,
+        ))
         self._enabled_extensions = tuple(options.enable_extensions)
-        self._all_selected = self._selected + self._enabled_extensions
-        self._ignored = tuple(options.ignore)
+        self._all_selected = tuple(sorted(
+            self._selected + self._enabled_extensions,
+            reverse=True,
+        ))
+        self._ignored = tuple(sorted(options.ignore, reverse=True))
         self._decision_cache = {}
         self._parsed_diff = {}
 
@@ -116,25 +122,21 @@ class StyleGuide(object):
 
     def _decision_for(self, code):
         # type: (Error) -> Decision
-        startswith = code.startswith
-        try:
-            selected = sorted([s for s in self._selected if startswith(s)])[0]
-        except IndexError:
-            selected = None
-        try:
-            ignored = sorted([i for i in self._ignored if startswith(i)])[0]
-        except IndexError:
-            ignored = None
-
-        if selected is None:
-            return Decision.Ignored
-
-        if ignored is None:
+        select = find_first_match(code, self._all_selected)
+        extra_select = find_first_match(code, self._extended_selected)
+        ignore = find_first_match(code, self._ignored)
+
+        if select and ignore:
+            return find_more_specific(select, ignore)
+        if extra_select and ignore:
+            return find_more_specific(extra_select, ignore)
+        if select or (extra_select and self._selected == defaults.SELECT):
             return Decision.Selected
-
-        if selected.startswith(ignored) and selected != ignored:
-            return Decision.Selected
-        return Decision.Ignored
+        if select is None and extra_select is None and ignore is not None:
+            return Decision.Ignored
+        if self._selected != defaults.SELECT and select is None:
+            return Decision.Ignored
+        return Decision.Selected
 
     def should_report_error(self, code):
         # type: (str) -> Decision
@@ -295,3 +297,19 @@ class StyleGuide(object):
             Dictionary mapping filenames to sets of line number ranges.
         """
         self._parsed_diff = diffinfo
+
+
+def find_more_specific(selected, ignored):
+    if selected.startswith(ignored) and selected != ignored:
+        return Decision.Selected
+    return Decision.Ignored
+
+
+def find_first_match(error_code, code_list):
+    startswith = error_code.startswith
+    for code in code_list:
+        if startswith(code):
+            break
+    else:
+        return None
+    return code
diff --git a/tests/unit/test_style_guide.py b/tests/unit/test_style_guide.py
index c11fd1d..7357b25 100644
--- a/tests/unit/test_style_guide.py
+++ b/tests/unit/test_style_guide.py
@@ -4,6 +4,7 @@ import optparse
 import mock
 import pytest
 
+from flake8 import defaults
 from flake8 import style_guide
 from flake8.formatting import base
 from flake8.plugins import notifier
@@ -135,6 +136,52 @@ def test_should_report_error(select_list, ignore_list, error_code, expected):
     assert guide.should_report_error(error_code) is expected
 
 
+@pytest.mark.parametrize(
+    'select,ignore,extend_select,enabled_extensions,error_code,expected', [
+        (defaults.SELECT, [], ['I1'], [], 'I100',
+            style_guide.Decision.Selected),
+        (defaults.SELECT, [], ['I1'], [], 'I201',
+            style_guide.Decision.Selected),
+        (defaults.SELECT, ['I2'], ['I1'], [], 'I101',
+            style_guide.Decision.Selected),
+        (defaults.SELECT, ['I2'], ['I1'], [], 'I201',
+            style_guide.Decision.Ignored),
+        (defaults.SELECT, ['I1'], ['I10'], [], 'I101',
+            style_guide.Decision.Selected),
+        (defaults.SELECT, ['I10'], ['I1'], [], 'I101',
+            style_guide.Decision.Ignored),
+        (defaults.SELECT, [], [], ['U4'], 'U401',
+            style_guide.Decision.Selected),
+        (defaults.SELECT, ['U401'], [], ['U4'], 'U401',
+            style_guide.Decision.Ignored),
+        (defaults.SELECT, ['U401'], [], ['U4'], 'U402',
+            style_guide.Decision.Selected),
+        (['E2'], ['E21'], [], [], 'E221', style_guide.Decision.Selected),
+        (['E2'], ['E21'], [], [], 'E212', style_guide.Decision.Ignored),
+        (['F', 'W'], ['C90'], ['I1'], [], 'C901',
+            style_guide.Decision.Ignored),
+    ]
+)
+def test_decision_for_logic(select, ignore, extend_select, enabled_extensions,
+                            error_code, expected):
+    """Verify the complicated logic of StyleGuide._decision_for.
+
+    I usually avoid testing private methods, but this one is very important in
+    our conflict resolution work in Flake8.
+    """
+    guide = style_guide.StyleGuide(
+        create_options(
+            select=select, ignore=ignore,
+            extended_default_select=extend_select,
+            enable_extensions=enabled_extensions,
+        ),
+        listener_trie=None,
+        formatter=None,
+    )
+
+    assert guide._decision_for(error_code) is expected
+
+
 @pytest.mark.parametrize('error_code,physical_line,expected_result', [
     ('E111', 'a = 1', False),
     ('E121', 'a = 1  # noqa: E111', False),