Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace "# type: T" comments with proper variable type annotations #904

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Feb 9, 2023

Now that we're using Python 3.8+ the more modern syntax is available to us, so use it. Also re-enable Pyright's reportTypeCommentUsage setting.

The second commit in this PR also removes a bunch of unneeded "noqa" and "pyright" comments.

I used a little script to find these and do a regex replace, then fixed up the few remaining ones (that were split on multiple lines) manually. Then I ran the following script to dump the AST (removing "annotated assignments") and did a diff against the original AST to ensure I hadn't messed up anything else.

import ast
import os

class Transformer(ast.NodeTransformer):
    def visit_AnnAssign(self, node):
        if node.value is None:
            return None
        return ast.Assign([node.target], node.value)

PATHS = """
ops/charm.py
ops/framework.py
ops/main.py
ops/model.py
ops/pebble.py
ops/storage.py
ops/testing.py
""".split()

for path in PATHS:
    with open(path) as f:
        source = f.read()
    filename = os.path.basename(path)
    tree = ast.parse(source, filename)

    transformed = Transformer().visit(tree)

    root, ext = os.path.splitext(filename)
    outname = f'ops/{root}.ast'
    print(outname)
    with open(outname, 'w') as f:
        f.write(ast.dump(tree, indent=4))

Now that we're using Python 3.8+ the more modern syntax is available to
us, so use it. Also re-enable Pyright's reportTypeCommentUsage setting.

I used a little script to find these and do a regex replace, then fixed
up the few remaining ones (that were split on multiple lines) manually.
Then I ran the following script to dump the AST (removing "annotated
assignments") and did a diff against the original AST to ensure I
hadn't messed up anything else.

```python
import ast
import os

class Transformer(ast.NodeTransformer):
    def visit_AnnAssign(self, node):
        if node.value is None:
            return None
        return ast.Assign([node.target], node.value)

PATHS = """
ops/charm.py
ops/framework.py
ops/main.py
ops/model.py
ops/pebble.py
ops/storage.py
ops/testing.py
""".split()

for path in PATHS:
    with open(path) as f:
        source = f.read()
    filename = os.path.basename(path)
    tree = ast.parse(source, filename)

    transformed = Transformer().visit(tree)

    root, ext = os.path.splitext(filename)
    outname = f'ops/{root}.ast'
    print(outname)
    with open(outname, 'w') as f:
        f.write(ast.dump(tree, indent=4))
```
@benhoyt benhoyt mentioned this pull request Feb 9, 2023
8 tasks
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

Nice! At long last :)

@benhoyt benhoyt merged commit 807ebc3 into canonical:main Feb 14, 2023
@benhoyt benhoyt deleted the remove-type-comments branch February 14, 2023 22:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants