From fe8586f3dcefca8b52e969e206077e52f16b73da Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 15:37:02 -0700 Subject: [PATCH 01/11] Adds JSON Schema for Fixit configs. --- fixit/cli/add_new_rule.py | 6 +-- fixit/common/config.py | 26 ++++------- fixit/common/config.schema.json | 52 +++++++++++++++++++++ fixit/common/tests/test_config.py | 70 +++++++++++++++++++++++++++++ fixit/common/unused_suppressions.py | 2 +- fixit/common/utils.py | 2 +- requirements.txt | 1 + 7 files changed, 137 insertions(+), 22 deletions(-) create mode 100644 fixit/common/config.schema.json create mode 100644 fixit/common/tests/test_config.py diff --git a/fixit/cli/add_new_rule.py b/fixit/cli/add_new_rule.py index 1b811f0b..27ca25d7 100644 --- a/fixit/cli/add_new_rule.py +++ b/fixit/cli/add_new_rule.py @@ -57,7 +57,7 @@ class {class_name}Rule(CstLintRule): def is_path_exists(path: str) -> Path: - """Check for valid path, if yes, return `Path` else raise `Error` """ + """Check for valid path, if yes, return `Path` else raise `Error`""" filepath = Path(path) if filepath.exists(): raise FileExistsError(f"{filepath} already exists") @@ -68,7 +68,7 @@ def is_path_exists(path: str) -> Path: def is_valid_name(name: str) -> str: - """Check for valid rule name """ + """Check for valid rule name""" if name.casefold().endswith(("rule", "rules")): raise ValueError("Please enter rule name without the suffix, 'rule' or 'Rule'") return snake_to_camelcase(name) @@ -89,7 +89,7 @@ def create_rule_file(file_path: Path, rule_name: str) -> None: def _add_arguments(parser: argparse.ArgumentParser) -> None: - """All required arguments for `add_new_rule` """ + """All required arguments for `add_new_rule`""" parser.add_argument( "--path", type=is_path_exists, diff --git a/fixit/common/config.py b/fixit/common/config.py index 47521bfd..92ee6a62 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -4,14 +4,17 @@ # LICENSE file in the root directory of this source tree. import distutils.spawn +import json import os import re +import sys from dataclasses import asdict from functools import lru_cache from pathlib import Path from typing import Any, Dict, Pattern, Set import yaml +from jsonschema import validate from fixit.common.base import LintConfig from fixit.common.utils import LintRuleCollectionT, import_distinct_rules_from_package @@ -70,22 +73,19 @@ def get_validated_settings( file_content: Dict[str, Any], current_dir: Path ) -> Dict[str, Any]: + # Validates the types and presence of the keys + cur_loc = os.path.realpath(os.path.join(current_dir, os.path.dirname(__file__))) + with open(os.path.join(cur_loc, "config.schema.json")) as f: + schema = json.load(f) + validate(instance=file_content, schema=schema) + settings = {} for list_setting_name in LIST_SETTINGS: if list_setting_name in file_content: - if not ( - isinstance(file_content[list_setting_name], list) - and all(isinstance(s, str) for s in file_content[list_setting_name]) - ): - raise TypeError( - f"Expected list of strings for `{list_setting_name}` setting." - ) settings[list_setting_name] = file_content[list_setting_name] for path_setting_name in PATH_SETTINGS: if path_setting_name in file_content: setting_value = file_content[path_setting_name] - if not isinstance(setting_value, str): - raise TypeError(f"Expected string for `{path_setting_name}` setting.") abspath: Path = (current_dir / setting_value).resolve() else: abspath: Path = current_dir @@ -95,17 +95,9 @@ def get_validated_settings( for nested_setting_name in NESTED_SETTINGS: if nested_setting_name in file_content: nested_setting = file_content[nested_setting_name] - if not isinstance(nested_setting, dict): - raise TypeError( - f"Expected key-value pairs for `{nested_setting_name}` setting." - ) settings[nested_setting_name] = {} # Verify that each setting is also a mapping for k, v in nested_setting.items(): - if not isinstance(v, dict): - raise TypeError( - f"Expected key-value pairs for `{v}` setting in {nested_setting_name}." - ) settings[nested_setting_name].update({k: v}) return settings diff --git a/fixit/common/config.schema.json b/fixit/common/config.schema.json new file mode 100644 index 00000000..6890ebd3 --- /dev/null +++ b/fixit/common/config.schema.json @@ -0,0 +1,52 @@ +{ + "$id": "config.schema.json", + "title": "FixIt Configuration File Schema", + "description": "Schema definition for FixIt configuration files", + "type": "object", + "properties": { + "block_list_patterns": { + "description": "A list of patterns that indicate that a file should not be linted.", + "type": "array", + "items": { + "type": "string" + } + }, + "block_list_rules": { + "description": "A list of rules (whether custom or from Fixit) that should not be applied to the repository.", + "type": "array", + "items": { + "type": "string" + } + }, + "fixture_dir": { + "description": "The directory in which fixture files required for unit testing are to be found.", + "type": "string" + }, + "use_noqa": { + "description": "Defaults to False. Use True to support Flake8 lint suppression comment: noqa.", + "type": "boolean" + }, + "formatter": { + "description": "A list of the formatter commands to use after a lint is complete.", + "type": "array", + "items": { + "type": "string" + } + }, + "packages": { + "description": "The Python packages in which to search for lint rules.", + "type": "array", + "items": { + "type": "string" + } + }, + "repo_root": { + "description": "The path to the repository root.", + "type": "string" + }, + "rule_config": { + "description": "Rule-specific configurations.", + "type": "object" + } + } +} diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py new file mode 100644 index 00000000..82d06deb --- /dev/null +++ b/fixit/common/tests/test_config.py @@ -0,0 +1,70 @@ +# Copyright (c) Facebook, Inc. and its affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +import os +from pathlib import Path + +from jsonschema.exceptions import ValidationError +from libcst.testing.utils import UnitTest + +from fixit.common.config import get_validated_settings + + +class TestConfig(UnitTest): + def test_validated_settings_with_bad_types(self) -> None: + bad_config = {"block_list_rules": False} + with self.assertRaises(ValidationError) as ex: + get_validated_settings(bad_config, Path(".")) + self.assertIn("False is not of type 'array'", str(ex.exception)) + + def test_validated_settings_with_correct_types(self) -> None: + config = {"block_list_rules": ["FakeRule"]} + settings = get_validated_settings(config, Path(".")) + self.assertEqual( + {"block_list_rules": ["FakeRule"], "fixture_dir": ".", "repo_root": "."}, + settings, + ) + + def test_validated_settings_all_keys(self) -> None: + self.maxDiff = None + config = { + "formatter": ["black", "-", "--no-diff"], + "packages": ["python.fixit.rules"], + "block_list_rules": ["Flake8PseudoLintRule"], + "fixture_dir": "/fake/path", + "repo_root": "/fake/path", + "rule_config": { + "UnusedImportsRule": { + "ignored_unused_modules": [ + "__future__", + "__static__", + "__static__.compiler_flags", + "__strict__", + ] + }, + "NoBadCallsRule": { + "badCalls": { + "calls": [ + "os.spawnl", + "os.spawnle", + "os.spawnlp", + "os.spawnlpe", + "os.spawnv", + "os.spawnve", + "os.spawnvp", + "os.spawnvpe", + ], + "why": """Using `os.spawn*` is deprecated. Use `subprocess` instead.\ + See https://www.python.org/dev/peps/pep-0324/#replacing-os-spawn for more\ + information.""", + } + }, + }, + } + settings = get_validated_settings(config, Path(".")) + self.assertEqual( + config, + settings, + ) diff --git a/fixit/common/unused_suppressions.py b/fixit/common/unused_suppressions.py index 888ad505..82c8f9c1 100644 --- a/fixit/common/unused_suppressions.py +++ b/fixit/common/unused_suppressions.py @@ -62,7 +62,7 @@ def _compose_new_comment( def _get_unused_codes_in_comment( local_supp_comment: SuppressionComment, ignored_rules_that_ran: Collection[str] ) -> Collection[str]: - """ Returns a subset of the rules in the comment which did not show up in any report. """ + """Returns a subset of the rules in the comment which did not show up in any report.""" return { ir for ir in ignored_rules_that_ran diff --git a/fixit/common/utils.py b/fixit/common/utils.py index 4c2c0ebe..9bcc7e6c 100644 --- a/fixit/common/utils.py +++ b/fixit/common/utils.py @@ -86,7 +86,7 @@ def expected_str(self) -> str: def import_submodules(package: str, recursive: bool = True) -> Dict[str, ModuleType]: - """ Import all submodules of a module, recursively, including subpackages. """ + """Import all submodules of a module, recursively, including subpackages.""" package: ModuleType = importlib.import_module(package) results = {} # pyre-fixme[16]: `ModuleType` has no attribute `__path__`. diff --git a/requirements.txt b/requirements.txt index 3755e13a..580ab1e1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ flake8>=3.8.1 libcst>=0.3.10 pyyaml>=5.2 +jsonschema>=3.2.0 From 44b79c0612219d2fe82ee93741e73d708804cffd Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 15:41:53 -0700 Subject: [PATCH 02/11] Removes self.maxDiff from test. --- fixit/common/tests/test_config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py index 82d06deb..6d52ce73 100644 --- a/fixit/common/tests/test_config.py +++ b/fixit/common/tests/test_config.py @@ -28,7 +28,6 @@ def test_validated_settings_with_correct_types(self) -> None: ) def test_validated_settings_all_keys(self) -> None: - self.maxDiff = None config = { "formatter": ["black", "-", "--no-diff"], "packages": ["python.fixit.rules"], From 5c9cca4505593ff7b62d3f7bb9eb9b4cc1e12d49 Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 15:52:48 -0700 Subject: [PATCH 03/11] Removes unused import. --- fixit/common/tests/test_config.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py index 6d52ce73..4de4aeb3 100644 --- a/fixit/common/tests/test_config.py +++ b/fixit/common/tests/test_config.py @@ -2,8 +2,6 @@ # # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. - -import os from pathlib import Path from jsonschema.exceptions import ValidationError @@ -43,23 +41,6 @@ def test_validated_settings_all_keys(self) -> None: "__strict__", ] }, - "NoBadCallsRule": { - "badCalls": { - "calls": [ - "os.spawnl", - "os.spawnle", - "os.spawnlp", - "os.spawnlpe", - "os.spawnv", - "os.spawnve", - "os.spawnvp", - "os.spawnvpe", - ], - "why": """Using `os.spawn*` is deprecated. Use `subprocess` instead.\ - See https://www.python.org/dev/peps/pep-0324/#replacing-os-spawn for more\ - information.""", - } - }, }, } settings = get_validated_settings(config, Path(".")) From 8e38d006de3bcffa44e6bbeebc80e9d2683e6fc8 Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 16:37:11 -0700 Subject: [PATCH 04/11] Removes another unused import. --- fixit/common/config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/fixit/common/config.py b/fixit/common/config.py index 92ee6a62..2a555090 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -7,7 +7,6 @@ import json import os import re -import sys from dataclasses import asdict from functools import lru_cache from pathlib import Path From 9c21a8ffb99d75e6167ae1d2267e22fa9e0b3ca0 Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 17:11:30 -0700 Subject: [PATCH 05/11] Adds config.schema.json to setup.py data files. --- MANIFEST.in | 2 +- fixit/common/config.py | 6 +++--- fixit/common/tests/test_config.py | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/MANIFEST.in b/MANIFEST.in index 76206f56..413e91f3 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1 +1 @@ -include README.rst LICENSE CODE_OF_CONDUCT.md CONTRIBUTING.md requirements.txt requirements-dev.txt fixit/py.typed +include README.rst LICENSE CODE_OF_CONDUCT.md CONTRIBUTING.md requirements.txt requirements-dev.txt fixit/py.typed config.schema.json diff --git a/fixit/common/config.py b/fixit/common/config.py index 2a555090..89c423b9 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -4,6 +4,7 @@ # LICENSE file in the root directory of this source tree. import distutils.spawn +import importlib.resources as pkg_resources import json import os import re @@ -73,9 +74,8 @@ def get_validated_settings( file_content: Dict[str, Any], current_dir: Path ) -> Dict[str, Any]: # Validates the types and presence of the keys - cur_loc = os.path.realpath(os.path.join(current_dir, os.path.dirname(__file__))) - with open(os.path.join(cur_loc, "config.schema.json")) as f: - schema = json.load(f) + config = pkg_resources.read_text(__package__, "config.schema.json") + schema = json.loads(config) validate(instance=file_content, schema=schema) settings = {} diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py index 4de4aeb3..40bc0dda 100644 --- a/fixit/common/tests/test_config.py +++ b/fixit/common/tests/test_config.py @@ -2,6 +2,7 @@ # # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. + from pathlib import Path from jsonschema.exceptions import ValidationError @@ -26,6 +27,7 @@ def test_validated_settings_with_correct_types(self) -> None: ) def test_validated_settings_all_keys(self) -> None: + self.maxDiff = None config = { "formatter": ["black", "-", "--no-diff"], "packages": ["python.fixit.rules"], From 2d0dc8997f9e529ce8e0200a80cad9b355ab22f0 Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 17:16:23 -0700 Subject: [PATCH 06/11] Fix for <3.6. --- fixit/common/config.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fixit/common/config.py b/fixit/common/config.py index 89c423b9..06844bd4 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -4,7 +4,13 @@ # LICENSE file in the root directory of this source tree. import distutils.spawn -import importlib.resources as pkg_resources + + +try: + import importlib.resources as pkg_resources +except ImportError: # For <=3.6 + import importlib_resources as pkg_resources + import json import os import re From 00c56a8422de9777256b0fc0ca9f565d7b43701b Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 27 Apr 2021 17:36:09 -0700 Subject: [PATCH 07/11] Fix for windows. --- fixit/common/tests/test_config.py | 2 -- requirements.txt | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py index 40bc0dda..6d347119 100644 --- a/fixit/common/tests/test_config.py +++ b/fixit/common/tests/test_config.py @@ -32,8 +32,6 @@ def test_validated_settings_all_keys(self) -> None: "formatter": ["black", "-", "--no-diff"], "packages": ["python.fixit.rules"], "block_list_rules": ["Flake8PseudoLintRule"], - "fixture_dir": "/fake/path", - "repo_root": "/fake/path", "rule_config": { "UnusedImportsRule": { "ignored_unused_modules": [ diff --git a/requirements.txt b/requirements.txt index 580ab1e1..ccb934d5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,3 +2,4 @@ flake8>=3.8.1 libcst>=0.3.10 pyyaml>=5.2 jsonschema>=3.2.0 +importlib-resources>=5.1.2 From 2606ce6922a64cf6a2363441f7283c50c2d41ddd Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Wed, 28 Apr 2021 11:19:50 -0700 Subject: [PATCH 08/11] Fixed pyre and docs breakage. --- fixit/common/config.py | 16 ++++++++++++---- fixit/common/tests/test_config.py | 3 +++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/fixit/common/config.py b/fixit/common/config.py index 06844bd4..6abb1b91 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -27,6 +27,7 @@ LINT_CONFIG_FILE_NAME: Path = Path(".fixit.config.yaml") +LINT_CONFIG_SCHEMA_NAME: str = "config.schema.json" # https://gitlab.com/pycqa/flake8/blob/9631dac52aa6ed8a3de9d0983c/src/flake8/defaults.py NOQA_INLINE_REGEXP: Pattern[str] = re.compile( @@ -79,10 +80,17 @@ def get_validated_settings( file_content: Dict[str, Any], current_dir: Path ) -> Dict[str, Any]: - # Validates the types and presence of the keys - config = pkg_resources.read_text(__package__, "config.schema.json") - schema = json.loads(config) - validate(instance=file_content, schema=schema) + try: + # __package__ should never be none (config.py should not be run directly) + # But use .get() to make pyre happy + pkg = globals().get("__package__") + assert pkg, "No package was found, config types not validated." + config = pkg_resources.read_text(pkg, "config.schema.json") + # Validates the types and presence of the keys + schema = json.loads(config) + validate(instance=file_content, schema=schema) + except (AssertionError, FileNotFoundError): + pass settings = {} for list_setting_name in LIST_SETTINGS: diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py index 6d347119..3618d31a 100644 --- a/fixit/common/tests/test_config.py +++ b/fixit/common/tests/test_config.py @@ -3,6 +3,7 @@ # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. +import os from pathlib import Path from jsonschema.exceptions import ValidationError @@ -32,6 +33,8 @@ def test_validated_settings_all_keys(self) -> None: "formatter": ["black", "-", "--no-diff"], "packages": ["python.fixit.rules"], "block_list_rules": ["Flake8PseudoLintRule"], + "fixture_dir": f"{os.getcwd()}", + "repo_root": f"{os.getcwd()}", "rule_config": { "UnusedImportsRule": { "ignored_unused_modules": [ From 55dc6dd742dc011bd4d8776171208839e1c90dab Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Wed, 28 Apr 2021 11:22:07 -0700 Subject: [PATCH 09/11] Uses the global variable. --- fixit/common/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixit/common/config.py b/fixit/common/config.py index 6abb1b91..a3a5ad94 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -85,7 +85,7 @@ def get_validated_settings( # But use .get() to make pyre happy pkg = globals().get("__package__") assert pkg, "No package was found, config types not validated." - config = pkg_resources.read_text(pkg, "config.schema.json") + config = pkg_resources.read_text(pkg, LINT_CONFIG_SCHEMA_NAME) # Validates the types and presence of the keys schema = json.loads(config) validate(instance=file_content, schema=schema) From 23670b2281935c12c4480625eac66f6a852721c8 Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Tue, 4 May 2021 13:28:23 -0700 Subject: [PATCH 10/11] Fixes MANIFEST file. Removes unneeded settings dict. --- MANIFEST.in | 2 +- fixit/common/config.py | 39 +++++++++++---------------------------- 2 files changed, 12 insertions(+), 29 deletions(-) diff --git a/MANIFEST.in b/MANIFEST.in index 413e91f3..64dcf850 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1 +1 @@ -include README.rst LICENSE CODE_OF_CONDUCT.md CONTRIBUTING.md requirements.txt requirements-dev.txt fixit/py.typed config.schema.json +include README.rst LICENSE CODE_OF_CONDUCT.md CONTRIBUTING.md requirements.txt requirements-dev.txt fixit/py.typed fixit/common/config.schema.json diff --git a/fixit/common/config.py b/fixit/common/config.py index a3a5ad94..9de79839 100644 --- a/fixit/common/config.py +++ b/fixit/common/config.py @@ -71,31 +71,22 @@ ) -LIST_SETTINGS = ["formatter", "block_list_patterns", "block_list_rules", "packages"] PATH_SETTINGS = ["repo_root", "fixture_dir"] -NESTED_SETTINGS = ["rule_config"] DEFAULT_FORMATTER = ["black", "-"] def get_validated_settings( file_content: Dict[str, Any], current_dir: Path ) -> Dict[str, Any]: - try: - # __package__ should never be none (config.py should not be run directly) - # But use .get() to make pyre happy - pkg = globals().get("__package__") - assert pkg, "No package was found, config types not validated." - config = pkg_resources.read_text(pkg, LINT_CONFIG_SCHEMA_NAME) - # Validates the types and presence of the keys - schema = json.loads(config) - validate(instance=file_content, schema=schema) - except (AssertionError, FileNotFoundError): - pass - - settings = {} - for list_setting_name in LIST_SETTINGS: - if list_setting_name in file_content: - settings[list_setting_name] = file_content[list_setting_name] + # __package__ should never be none (config.py should not be run directly) + # But use .get() to make pyre happy + pkg = globals().get("__package__") + assert pkg, "No package was found, config types not validated." + config = pkg_resources.read_text(pkg, LINT_CONFIG_SCHEMA_NAME) + # Validates the types and presence of the keys + schema = json.loads(config) + validate(instance=file_content, schema=schema) + for path_setting_name in PATH_SETTINGS: if path_setting_name in file_content: setting_value = file_content[path_setting_name] @@ -103,17 +94,9 @@ def get_validated_settings( else: abspath: Path = current_dir # Set path setting to absolute path. - settings[path_setting_name] = str(abspath) - - for nested_setting_name in NESTED_SETTINGS: - if nested_setting_name in file_content: - nested_setting = file_content[nested_setting_name] - settings[nested_setting_name] = {} - # Verify that each setting is also a mapping - for k, v in nested_setting.items(): - settings[nested_setting_name].update({k: v}) + file_content[path_setting_name] = str(abspath) - return settings + return file_content @lru_cache() From 0bff6fc5771d04ceb962b492bbf9441d3908305d Mon Sep 17 00:00:00 2001 From: Lisa Roach Date: Wed, 5 May 2021 17:07:45 -0700 Subject: [PATCH 11/11] Updates unit test to use assertRaisesRegex. --- fixit/common/tests/test_config.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fixit/common/tests/test_config.py b/fixit/common/tests/test_config.py index 3618d31a..8adbe76b 100644 --- a/fixit/common/tests/test_config.py +++ b/fixit/common/tests/test_config.py @@ -15,9 +15,8 @@ class TestConfig(UnitTest): def test_validated_settings_with_bad_types(self) -> None: bad_config = {"block_list_rules": False} - with self.assertRaises(ValidationError) as ex: + with self.assertRaisesRegex(ValidationError, "False is not of type 'array'"): get_validated_settings(bad_config, Path(".")) - self.assertIn("False is not of type 'array'", str(ex.exception)) def test_validated_settings_with_correct_types(self) -> None: config = {"block_list_rules": ["FakeRule"]}