From 370912988f4524a15f513cafd4234822721893b7 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Wed, 20 Jul 2016 08:12:45 -0500 Subject: [PATCH 1/5] Document flake8-polyfill in compatibility section Yesterday we released the flake8-polyfill package to help with Flake8 compatibility issues. This adds documentation to Flake8 to help people use that and to guide them towards it. --- .../cross-compatibility.rst | 121 +++++++++++------- docs/source/requirements.txt | 1 + 2 files changed, 77 insertions(+), 45 deletions(-) diff --git a/docs/source/plugin-development/cross-compatibility.rst b/docs/source/plugin-development/cross-compatibility.rst index e1c7cf6..61ee3c2 100644 --- a/docs/source/plugin-development/cross-compatibility.rst +++ b/docs/source/plugin-development/cross-compatibility.rst @@ -12,10 +12,11 @@ versions. If your plugin does not register options, it *should* Just Work. -The **only** breaking change in |Flake8| 3.0 is the fact that we no longer -check the option parser for a list of strings to parse from a config file. On -|Flake8| 2.x, to have an option parsed from the configuration files that -|Flake8| finds and parses you would have to do something like: +The **only two** breaking changes in |Flake8| 3.0 is the fact that we no +longer check the option parser for a list of strings to parse from a config +file and we no longer patch pep8 or pycodestyle's ``stdin_get_value`` +functions. On |Flake8| 2.x, to have an option parsed from the configuration +files that |Flake8| finds and parses you would have to do something like: .. code-block:: python @@ -101,56 +102,86 @@ as a single path. Option Handling on Flake8 2 and 3 ================================= -So, in conclusion, we can now write our plugin that relies on registering -options with |Flake8| and have it work on |Flake8| 2.x and 3.x. +To ease the transition, the |Flake8| maintainers have released +`flake8-polyfill`_. |polyfill| provides a convenience function to help users +transition between Flake8 2 and 3 without issue. For example, if your plugin +has to work on Flake8 2.x and 3.x but you want to take advantage of some of +the new options to ``add_option``, you can do .. code-block:: python - import optparse - - option_args = ('-X', '--example-flag') - option_kwargs = { - 'type': 'string', - 'parse_from_config': True, - 'help': '...', - } - try: - # Flake8 3.x registration - parser.add_option(*option_args, **option_kwargs) - except (optparse.OptionError, TypeError): - # Flake8 2.x registration - parse_from_config = option_kwargs.pop('parse_from_config', False) - option = parser.add_option(*option_args, **option_kwargs) - if parse_from_config: - parser.config_options.append(option.get_opt_string().lstrip('-')) + from flake8_polyfill import options -Or, you can write a tiny helper function: + class MyPlugin(object): + @classmethod + def add_options(cls, parser): + options.register( + parser, + '--application-names', default='', type='string', + help='Names of the applications to be checked.', + parse_from_config=True, + comma_separated_list=True, + ) + options.register( + parser, + '--style-name', default='', type='string', + help='The name of the style convention you want to use', + parse_from_config=True, + ) + options.register( + parser, + '--application-paths', default='', type='string', + help='Locations of the application code', + parse_from_config=True, + comma_separated_list=True, + normalize_paths=True, + ) + + @classmethod + def parse_options(cls, parsed_options): + cls.application_names = parsed_options.application_names + cls.style_name = parsed_options.style_name + cls.application_paths = parsed_options.application_paths + +|polyfill| will handle these extra options using *callbacks* to the option +parser. The project has direct replications of the functions that |Flake8| +uses to provide the same functionality. This means that the values you receive +should be identically parsed whether you're using Flake8 2.x or 3.x. + +.. autofunction:: flake8_polyfill.options.register + + +Standard In Handling on Flake8 2.5, 2.6, and 3 +============================================== + +After releasing |Flake8| 2.6, handling standard-in became a bit trickier for +some plugins. |Flake8| 2.5 and earlier had started monkey-patching pep8's +``stdin_get_value`` function. 2.6 switched to pycodestyle and only +monkey-patched that. 3.0 has its own internal implementation and uses that but +does not directly provide anything for plugins using pep8 and pycodestyle's +``stdin_get_value`` function. |polyfill| provides this functionality for +plugin developers via it's :mod:`flake8_polyfill.stdin` module. + +If a plugin needs to read the content from stdin, it can do the following: .. code-block:: python - import optparse + from flake8_polyfill import stdin - def register_opt(parser, *args, **kwargs): - try: - # Flake8 3.x registration - parser.add_option(*args, **kwargs) - except (optparse.OptionError, TypeError): - # Flake8 2.x registration - parse_from_config = kwargs.pop('parse_from_config', False) - kwargs.pop('comma_separated_list', False) - kwargs.pop('normalize_paths', False) - option = parser.add_option(*args, **kwargs) - if parse_from_config: - parser.config_options.append(option.get_opt_string().lstrip('-')) + stdin.monkey_patch('pep8') # To monkey-patch only pep8 + stdin.monkey_patch('pycodestyle') # To monkey-patch only pycodestyle + stdin.monkey_patch('all') # To monkey-patch both pep8 and pycodestyle -.. code-block:: python - @classmethod - def register_options(cls, parser): - register_opt(parser, '-X', '--example-flag', type='string', - parse_from_config=True, help='...') +Further, when using ``all``, |polyfill| does not require both packages to be +installed but will attempt to monkey-patch both and will silently ignore the +fact that pep8 or pycodestyle is not installed. -The transition period is admittedly not fantastic, but we believe that this -is a worthwhile change for plugin developers going forward. We also hope to -help with the transition phase for as many plugins as we can manage. +.. autofunction:: flake8_polyfill.stdin.monkey_patch + + +.. links +.. _flake8-polyfill: https://pypi.io/project/flake8-polyfill/ + +.. |polyfill| replace:: ``flake8-polyfill`` diff --git a/docs/source/requirements.txt b/docs/source/requirements.txt index 77bd874..4b5907f 100644 --- a/docs/source/requirements.txt +++ b/docs/source/requirements.txt @@ -2,3 +2,4 @@ sphinx>=1.3.0 sphinx_rtd_theme sphinx-prompt configparser +flake8-polyfill From 8a2e7ff908a6bf0361972a6369a07dc3bef2d373 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Wed, 20 Jul 2016 08:17:33 -0500 Subject: [PATCH 2/5] Trim trailing whitespace in compat docs --- docs/source/plugin-development/cross-compatibility.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/plugin-development/cross-compatibility.rst b/docs/source/plugin-development/cross-compatibility.rst index 61ee3c2..9cf38b8 100644 --- a/docs/source/plugin-development/cross-compatibility.rst +++ b/docs/source/plugin-development/cross-compatibility.rst @@ -12,7 +12,7 @@ versions. If your plugin does not register options, it *should* Just Work. -The **only two** breaking changes in |Flake8| 3.0 is the fact that we no +The **only two** breaking changes in |Flake8| 3.0 is the fact that we no longer check the option parser for a list of strings to parse from a config file and we no longer patch pep8 or pycodestyle's ``stdin_get_value`` functions. On |Flake8| 2.x, to have an option parsed from the configuration From b2b4cae8e3ef27b8545a8d98a35dc7b07b1b132f Mon Sep 17 00:00:00 2001 From: Leonardo Rochael Almeida Date: Wed, 20 Jul 2016 16:43:34 -0300 Subject: [PATCH 3/5] Allow stdin and directly named files to be excluded from check For the sake of IDEs, check filename for exclusion even if the file is directly named in the command line. Also, if the filename is "-" (stdin) check the provided display name for exclusion. Also, avoid calling path checking functions on the "-" filename: * fnmatch.fnmatch() * os.path.isdir() * os.path.exists() --- src/flake8/checker.py | 15 ++++++++++----- src/flake8/utils.py | 26 ++++++++++++++------------ 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/flake8/checker.py b/src/flake8/checker.py index f7bd540..d5132ff 100644 --- a/src/flake8/checker.py +++ b/src/flake8/checker.py @@ -237,6 +237,12 @@ class Manager(object): exclude = self.options.exclude if not exclude: return False + if path == '-': + # stdin, use display name to check exclusion, if present + path = self.options.stdin_display_name + if path is None: + LOG.debug("unnamed stdin has not been excluded") + return False basename = os.path.basename(path) if utils.fnmatch(basename, exclude): LOG.debug('"%s" has been excluded', basename) @@ -263,12 +269,11 @@ class Manager(object): # best solution right now. def should_create_file_checker(filename): """Determine if we should create a file checker.""" - matches_filename_patterns = utils.fnmatch( - filename, filename_patterns + return ( + filename == '-' or # stdin + utils.fnmatch(filename, filename_patterns) and + os.path.exists(filename) ) - is_stdin = filename == '-' - file_exists = os.path.exists(filename) - return (file_exists and matches_filename_patterns) or is_stdin self.checkers = [ FileChecker(filename, self.checks, self.style_guide) diff --git a/src/flake8/utils.py b/src/flake8/utils.py index 68ed530..52417fc 100644 --- a/src/flake8/utils.py +++ b/src/flake8/utils.py @@ -208,24 +208,26 @@ def filenames_from(arg, predicate=None): """ if predicate is None: predicate = _default_predicate - if os.path.isdir(arg): - for root, sub_directories, files in os.walk(arg): - if predicate(root): - sub_directories[:] = [] - continue + if predicate(arg): + return + + if arg == "-": + # stdin, don't call isdir() + yield arg + elif os.path.isdir(arg): + for root, sub_directories, files in os.walk(arg): # NOTE(sigmavirus24): os.walk() will skip a directory if you # remove it from the list of sub-directories. - for directory in sub_directories: - joined = os.path.join(root, directory) - if predicate(directory) or predicate(joined): - sub_directories.remove(directory) + sub_directories[:] = [ + directory for directory in sub_directories + if not predicate(os.path.join(root, directory)) + ] for filename in files: joined = os.path.join(root, filename) - if predicate(joined) or predicate(filename): - continue - yield joined + if not predicate(joined): + yield joined else: yield arg From 7934f8dce2fc1c5d8da374a3c7435d36f9526b0b Mon Sep 17 00:00:00 2001 From: Leonardo Rochael Almeida Date: Wed, 20 Jul 2016 17:22:49 -0300 Subject: [PATCH 4/5] Propagate the stdin_display_name to checker and processor This way plugins like flake8-putty can have access to the correct filename. --- src/flake8/checker.py | 8 ++++---- src/flake8/main/options.py | 2 +- src/flake8/processor.py | 17 ++++++++++------- src/flake8/style_guide.py | 2 -- tests/unit/test_file_processor.py | 7 ++++++- 5 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/flake8/checker.py b/src/flake8/checker.py index d5132ff..7c70ace 100644 --- a/src/flake8/checker.py +++ b/src/flake8/checker.py @@ -398,20 +398,20 @@ class FileChecker(object): :type style_guide: flake8.style_guide.StyleGuide """ - self.filename = filename self.checks = checks self.style_guide = style_guide self.results = [] - self.processor = self._make_processor() + self.processor = self._make_processor(filename) + self.filename = self.processor.filename self.statistics = { 'tokens': 0, 'logical lines': 0, 'physical lines': len(self.processor.lines), } - def _make_processor(self): + def _make_processor(self, filename): try: - return processor.FileProcessor(self.filename, + return processor.FileProcessor(filename, self.style_guide.options) except IOError: # If we can not read the file due to an IOError (e.g., the file diff --git a/src/flake8/main/options.py b/src/flake8/main/options.py index c725c38..dbfdf9b 100644 --- a/src/flake8/main/options.py +++ b/src/flake8/main/options.py @@ -74,7 +74,7 @@ def register_default_options(option_manager): ) add_option( - '--stdin-display-name', default='stdin', + '--stdin-display-name', help='The name used when reporting errors from code passed via stdin.' ' This is useful for editors piping the file contents to flake8.' ' (Default: %default)', diff --git a/src/flake8/processor.py b/src/flake8/processor.py index 76c5512..ae82b50 100644 --- a/src/flake8/processor.py +++ b/src/flake8/processor.py @@ -55,12 +55,13 @@ class FileProcessor(object): :param str filename: Name of the file to process """ + self.options = options self.filename = filename self.lines = lines if lines is None: - self.lines = self.read_lines() + # allow for stdin filename substitution + self.filename, self.lines = self.read_lines(filename) self.strip_utf_bom() - self.options = options # Defaults for public attributes #: Number of preceding blank lines @@ -268,13 +269,15 @@ class FileProcessor(object): self.indent_char = line[0] return line - def read_lines(self): + def read_lines(self, filename): # type: () -> List[str] """Read the lines for this file checker.""" - if self.filename is None or self.filename == '-': - self.filename = 'stdin' - return self.read_lines_from_stdin() - return self.read_lines_from_filename() + if filename is None or filename == '-': + filename = self.options.stdin_display_name or 'stdin' + lines = self.read_lines_from_stdin() + else: + lines = self.read_lines_from_filename() + return (filename, lines) def _readlines_py2(self): # type: () -> List[str] diff --git a/src/flake8/style_guide.py b/src/flake8/style_guide.py index ed1b844..284444d 100644 --- a/src/flake8/style_guide.py +++ b/src/flake8/style_guide.py @@ -260,8 +260,6 @@ class StyleGuide(object): """ error = Error(code, filename, line_number, column_number, text, physical_line) - if error.filename is None or error.filename == '-': - error = error._replace(filename=self.options.stdin_display_name) error_is_selected = (self.should_report_error(error.code) is Decision.Selected) is_not_inline_ignored = self.is_inline_ignored(error) is False diff --git a/tests/unit/test_file_processor.py b/tests/unit/test_file_processor.py index dec1667..8bc0e2d 100644 --- a/tests/unit/test_file_processor.py +++ b/tests/unit/test_file_processor.py @@ -14,6 +14,7 @@ def options_from(**kwargs): kwargs.setdefault('hang_closing', True) kwargs.setdefault('max_line_length', 79) kwargs.setdefault('verbose', False) + kwargs.setdefault('stdin_display_name', None) return optparse.Values(kwargs) @@ -63,13 +64,17 @@ def test_read_lines_from_stdin(stdin_get_value): @mock.patch('flake8.utils.stdin_get_value') -def test_read_lines_sets_filename_attribute(stdin_get_value): +def test_stdin_filename_attribute(stdin_get_value): """Verify that we update the filename attribute.""" stdin_value = mock.Mock() stdin_value.splitlines.return_value = [] stdin_get_value.return_value = stdin_value file_processor = processor.FileProcessor('-', options_from()) assert file_processor.filename == 'stdin' + file_processor = processor.FileProcessor('-', options_from( + stdin_display_name="foo.py" + )) + assert file_processor.filename == 'foo.py' def test_line_for(): From a1fdb5a2b5934ef5629df2335b2d495ba5252812 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Wed, 20 Jul 2016 19:28:13 -0500 Subject: [PATCH 5/5] Fix up merge request 78 This simplifies the changes, reduces the scope of refactors apparently for refactoring's sake and ensures that the internals are reasonable. It also airs on the side of preserving information rather than discarding or overwriting it. --- src/flake8/checker.py | 50 +++++++++++++++---------------- src/flake8/main/options.py | 2 +- src/flake8/processor.py | 11 ++++--- src/flake8/utils.py | 22 +++++++------- tests/unit/test_file_processor.py | 14 +++++++-- 5 files changed, 54 insertions(+), 45 deletions(-) diff --git a/src/flake8/checker.py b/src/flake8/checker.py index 7c70ace..ba412e3 100644 --- a/src/flake8/checker.py +++ b/src/flake8/checker.py @@ -234,15 +234,14 @@ class Manager(object): :rtype: bool """ + if path == '-': + if self.options.stdin_display_name == 'stdin': + return False + path = self.options.stdin_display_name + exclude = self.options.exclude if not exclude: return False - if path == '-': - # stdin, use display name to check exclusion, if present - path = self.options.stdin_display_name - if path is None: - LOG.debug("unnamed stdin has not been excluded") - return False basename = os.path.basename(path) if utils.fnmatch(basename, exclude): LOG.debug('"%s" has been excluded', basename) @@ -269,14 +268,15 @@ class Manager(object): # best solution right now. def should_create_file_checker(filename): """Determine if we should create a file checker.""" - return ( - filename == '-' or # stdin - utils.fnmatch(filename, filename_patterns) and - os.path.exists(filename) + matches_filename_patterns = utils.fnmatch( + filename, filename_patterns ) + is_stdin = filename == '-' + file_exists = os.path.exists(filename) + return (file_exists and matches_filename_patterns) or is_stdin self.checkers = [ - FileChecker(filename, self.checks, self.style_guide) + FileChecker(filename, self.checks, self.options) for argument in paths for filename in utils.filenames_from(argument, self.is_path_excluded) @@ -299,7 +299,7 @@ class Manager(object): results_reported = results_found = 0 for checker in self.checkers: results = sorted(checker.results, key=lambda tup: (tup[2], tup[3])) - results_reported += self._handle_results(checker.filename, + results_reported += self._handle_results(checker.display_name, results) results_found += len(results) return (results_found, results_reported) @@ -320,9 +320,9 @@ class Manager(object): final_results[filename] = results for checker in self.checkers: - filename = checker.filename + filename = checker.display_name checker.results = sorted(final_results.get(filename, []), - key=lambda tup: (tup[1], tup[2])) + key=lambda tup: (tup[2], tup[2])) def run_serial(self): """Run the checkers in serial.""" @@ -384,7 +384,7 @@ class Manager(object): class FileChecker(object): """Manage running checks for a file and aggregate the results.""" - def __init__(self, filename, checks, style_guide): + def __init__(self, filename, checks, options): """Initialize our file checker. :param str filename: @@ -393,26 +393,26 @@ class FileChecker(object): The plugins registered to check the file. :type checks: flake8.plugins.manager.Checkers - :param style_guide: - The initialized StyleGuide for this particular run. - :type style_guide: - flake8.style_guide.StyleGuide + :param options: + Parsed option values from config and command-line. + :type options: + optparse.Values """ + self.options = options + self.filename = filename self.checks = checks - self.style_guide = style_guide self.results = [] - self.processor = self._make_processor(filename) - self.filename = self.processor.filename + self.processor = self._make_processor() + self.display_name = self.processor.filename self.statistics = { 'tokens': 0, 'logical lines': 0, 'physical lines': len(self.processor.lines), } - def _make_processor(self, filename): + def _make_processor(self): try: - return processor.FileProcessor(filename, - self.style_guide.options) + return processor.FileProcessor(self.filename, self.options) except IOError: # If we can not read the file due to an IOError (e.g., the file # does not exist or we do not have the permissions to open it) diff --git a/src/flake8/main/options.py b/src/flake8/main/options.py index dbfdf9b..c725c38 100644 --- a/src/flake8/main/options.py +++ b/src/flake8/main/options.py @@ -74,7 +74,7 @@ def register_default_options(option_manager): ) add_option( - '--stdin-display-name', + '--stdin-display-name', default='stdin', help='The name used when reporting errors from code passed via stdin.' ' This is useful for editors piping the file contents to flake8.' ' (Default: %default)', diff --git a/src/flake8/processor.py b/src/flake8/processor.py index ae82b50..3e7dba5 100644 --- a/src/flake8/processor.py +++ b/src/flake8/processor.py @@ -59,8 +59,7 @@ class FileProcessor(object): self.filename = filename self.lines = lines if lines is None: - # allow for stdin filename substitution - self.filename, self.lines = self.read_lines(filename) + self.lines = self.read_lines() self.strip_utf_bom() # Defaults for public attributes @@ -269,15 +268,15 @@ class FileProcessor(object): self.indent_char = line[0] return line - def read_lines(self, filename): + def read_lines(self): # type: () -> List[str] """Read the lines for this file checker.""" - if filename is None or filename == '-': - filename = self.options.stdin_display_name or 'stdin' + if self.filename is None or self.filename == '-': + self.filename = self.options.stdin_display_name lines = self.read_lines_from_stdin() else: lines = self.read_lines_from_filename() - return (filename, lines) + return lines def _readlines_py2(self): # type: () -> List[str] diff --git a/src/flake8/utils.py b/src/flake8/utils.py index 52417fc..7f31f8f 100644 --- a/src/flake8/utils.py +++ b/src/flake8/utils.py @@ -212,22 +212,24 @@ def filenames_from(arg, predicate=None): if predicate(arg): return - if arg == "-": - # stdin, don't call isdir() - yield arg - elif os.path.isdir(arg): + if os.path.isdir(arg): for root, sub_directories, files in os.walk(arg): + if predicate(root): + sub_directories[:] = [] + continue + # NOTE(sigmavirus24): os.walk() will skip a directory if you # remove it from the list of sub-directories. - sub_directories[:] = [ - directory for directory in sub_directories - if not predicate(os.path.join(root, directory)) - ] + for directory in sub_directories: + joined = os.path.join(root, directory) + if predicate(directory) or predicate(joined): + sub_directories.remove(directory) for filename in files: joined = os.path.join(root, filename) - if not predicate(joined): - yield joined + if predicate(joined) or predicate(filename): + continue + yield joined else: yield arg diff --git a/tests/unit/test_file_processor.py b/tests/unit/test_file_processor.py index 8bc0e2d..547d6a0 100644 --- a/tests/unit/test_file_processor.py +++ b/tests/unit/test_file_processor.py @@ -14,7 +14,7 @@ def options_from(**kwargs): kwargs.setdefault('hang_closing', True) kwargs.setdefault('max_line_length', 79) kwargs.setdefault('verbose', False) - kwargs.setdefault('stdin_display_name', None) + kwargs.setdefault('stdin_display_name', 'stdin') return optparse.Values(kwargs) @@ -71,10 +71,18 @@ def test_stdin_filename_attribute(stdin_get_value): stdin_get_value.return_value = stdin_value file_processor = processor.FileProcessor('-', options_from()) assert file_processor.filename == 'stdin' + + +@mock.patch('flake8.utils.stdin_get_value') +def test_read_lines_uses_display_name(stdin_get_value): + """Verify that when processing stdin we use a display name if present.""" + stdin_value = mock.Mock() + stdin_value.splitlines.return_value = [] + stdin_get_value.return_value = stdin_value file_processor = processor.FileProcessor('-', options_from( - stdin_display_name="foo.py" + stdin_display_name='display_name.py' )) - assert file_processor.filename == 'foo.py' + assert file_processor.filename == 'display_name.py' def test_line_for():