Conversation
rachelzUT
changed the title
Coverage Report for CI Build 36151586431Coverage decreased (-0.1%) to 90.78%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
@rachelzUT great work, I know this was a significant effort. I left some inline comments, some of which apply broadly throughout the changes (e.g. the reporter type and some style things).
Also, in general prefer using mypy ignore comments over calling cast to avoid individual errors. In the future, I'll review the ignore comments separately to determine the best way to handle them, possibly with code changes.
| m = sys.modules["__main__"] | ||
| spec = importlib.util.spec_from_file_location(m.__name__, m.__file__) | ||
| mod = spec.origin | ||
| if spec is None: |
There was a problem hiding this comment.
Let's simplify this a bit, you should be able to do something like if spec is None or spec.origin is None
| return | ||
|
|
||
| eval_params = [const.value for const in inferred_params] | ||
| eval_params = [ |
There was a problem hiding this comment.
This addition is okay, but let's combine it with the check on Lines 36-37. You can check to see whether len(eval_params) == len(inferred_params).
| and isinstance(stop_arg.args[0], nodes.Name) | ||
| ): | ||
| return stop_arg.args[0].name | ||
| return None |
There was a problem hiding this comment.
Put this return inside an else branch
| return os.path.join(curr_dir, "config", "pylintrc") | ||
| elif os.path.exists(os.path.join(curr_dir, "config", "pyproject.toml")): | ||
| return os.path.join(curr_dir, "config", "pyproject.toml") | ||
| return None |
There was a problem hiding this comment.
Put this return statement inside an else branch
| args_list.extend(pylint_args) | ||
| _config_initialization(linter, args_list=args_list, config_file=config_location) | ||
| linter.config_file = config_location | ||
| setattr(linter, "config_file", config_location) # use setattr to avoid mypy errors |
There was a problem hiding this comment.
Use a mypy ignore comment instead of modifying the code
|
|
||
| def _add_parens(source_code): | ||
| def h(node): | ||
| def _add_parens(source_code: list[str]) -> Callable[[nodes.NodeNG], nodes.NodeNG]: |
There was a problem hiding this comment.
Use NodeTransform for the return type
| return ast | ||
|
|
||
| linter.get_ast = new_get_ast | ||
| cast(Any, linter).get_ast = new_get_ast |
There was a problem hiding this comment.
Use a mypy ignore comment
| reporter = _invoke_checker(checker, [temp_file.name], config, output_format) | ||
| # Clean up the temporary file | ||
| path.os.unlink(temp_file.name) | ||
| # Clean up the temporary file. |
| messages_config_path = linter.config.messages_config_path | ||
| # Pylint exposes a reporter object dynamically; PythonTA only relies on the | ||
| # BaseReporter API here, so narrow it for the rest of this helper. | ||
| current_reporter = cast(BaseReporter, linter.reporter) |
There was a problem hiding this comment.
There's some awkwardness in general about our treatment of reporter types. I think we can simplify by enforcing that we only use subclasses of PythonTaReporter (rather than pylint's `BaseReporter).
This won't help with attributes on linter, but I hope it helps with using the reporter later.
| self._control_boundaries = [] | ||
| self.z3_enabled = z3_enabled | ||
|
|
||
| def _cb(self) -> CFGBlock: |
There was a problem hiding this comment.
Overall I would prefer not changing all uses of _current_block and _current_cfg.
Instead of defining these functions, please add assert statements to each of the visit_* methods (other than visit_module, which is where these attributes are set to non-None values)
This branch has not been deployed
3 participants
Proposed Changes
Added type annotations to the
python-tacodebase, particularly to thepackages\python-ta\src\python_tadirectory.mypywas run on all files in this directory and fixes were implemented to resolve all type checking errors reported bymypy. This was done in efforts to improve code quality, and for aiding the inclusion ofmypyto theprekpre-commit hook configuration which will runmypyon all future commits.Additional notes:
pyproject.tomlfile was used to add configuration options formypy, specifically to ignore missing library stubs or py.typed markersmypyerrors....
Screenshots of your changes (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
Type annotations and
mypyfixes were made only to thepackages\python-ta\src\python_tadirectory. I believe making similar edits and fixes to the test files inpackages\python-ta\testswould be difficult as many of them purposely introduce issues such as incorrect formatting and type annotations for the purpose of testing checkers and related features.