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

Replaced all instances of outdated type annotations (List, Dict, etc.…) #1081

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

CulmoneY
Copy link

@CulmoneY CulmoneY commented Sep 14, 2024

Proposed Changes

Updated the PythonTA codebase to use modern type annotations. Specifically, I replaced List with list, Dict with dict, Set with set, and Tuple with tuple. I addressed the following two requirements:

  1. Added from __future__ import annotations to the top of each file where these type annotations are used, if it wasn't present already.
  2. Removed any unused imports of List, Dict, Set, and Tuple to clean up the codebase.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
♻️ Refactoring (internal change to codebase, without changing functionality) X
🚦 Test update (change that only adds or modifies tests)
📚 Documentation update (change that only updates documentation)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

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:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
  • I have updated the project documentation, if applicable.
    • This is required for new features.
  • I have updated the project Changelog (this is required for all changes).
  • If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

  • I have verified that the pre-commit.ci checks have passed.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported by Coveralls.
  • I have requested a review from a project maintainer.

…) with the current built-in types (list, dict, etc.) .
Copy link
Contributor

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CulmoneY okay good work, though overall you were a bit too broad with your find-and-replace (see inline comments).

In addition to the comments I left inline, I have two additional comments:

  1. You'll need to update your branch since I merged in @Raine-Yang-UofT's PR (see my message on Slack). There will be merge conflicts, so you have a good opportunity to practice resolving those.
  2. Update the Changelog. This is an internal change.

docs/checkers/index.md Outdated Show resolved Hide resolved
examples/ending_locations/subscript.py Outdated Show resolved Hide resolved
examples/ending_locations/subscript.py Show resolved Hide resolved
python_ta/checkers/forbidden_io_function_checker.py Outdated Show resolved Hide resolved
python_ta/checkers/forbidden_python_syntax_checker.py Outdated Show resolved Hide resolved
tests/test_cfg/test_cfg_generator.py Outdated Show resolved Hide resolved
tests/test_config/file_fixtures/test.pylintrc Show resolved Hide resolved
tests/test_contracts/test_class_forward_reference.py Outdated Show resolved Hide resolved
tests/test_custom_checkers/test_type_annotation_checker.py Outdated Show resolved Hide resolved
@david-yz-liu
Copy link
Contributor

@CulmoneY oh sorry I realized you hadn't yet requested a review, so I may have been a bit premature! Well, keep working :)

@CulmoneY CulmoneY marked this pull request as draft September 15, 2024 20:23
@CulmoneY CulmoneY marked this pull request as ready for review September 15, 2024 21:22
@CulmoneY CulmoneY marked this pull request as draft September 16, 2024 01:21
@CulmoneY CulmoneY marked this pull request as ready for review September 16, 2024 01:21
@@ -29,12 +31,12 @@

DEBUG_CONTRACTS = False
"""
Set to True to display debugging messages when checking contracts.
set to True to display debugging messages when checking contracts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be reverted (as below)

>>> from typing import List
>>> _display_annotation(List[int])
'typing.List[int]'
>>> from typing import list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire doctest example needs to be updated (note that you can, and should, run the doctest example manually to ensure it passes)

@@ -478,7 +478,7 @@ def end_setter_from_source(source_code, pred, only_consumables=False):
def set_endings_from_source(node):
# Tuple nodes have an end_col_offset that includes the end paren,
# but their col_offset does not include the start paren.
# To address this, we override the Tuple node's end_col_offset.
# To address this, we override the tuple node's end_col_offset.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this change

) -> List[z3.ExprRef]:
"""Convert an astroid List, Set, Tuple node to a list of z3 expressions."""
) -> list[z3.ExprRef]:
"""Convert an astroid list, Set, tuple node to a list of z3 expressions."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert these changes

@@ -43,7 +43,7 @@ def create_cfg_funcs_only():

This fixture specifies that cfgs will only be created for functions.
"""
# Setup: store the paths of the files being used/created
# setup: store the paths of the files being used/created
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this change

tests/test_config/file_fixtures/test.pylintrc Show resolved Hide resolved
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