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.

Sign up for GitHub

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

Jump to bottom

Type hint sybil/example.py #78

Merged
merged 2 commits into from
Sep 9, 2023
Merged

Type hint sybil/example.py #78

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9446e66
Type hint sybil/example.py
adamtheturtle Sep 4, 2023
424c6e6
Correctly type self.document.evaluator
adamtheturtle Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions sybil/example.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import TYPE_CHECKING, Any
from typing import TYPE_CHECKING, Any, Dict

from .region import Region

Expand Down Expand Up @@ -26,7 +26,7 @@ class Example:
"""

def __init__(
self, document: 'Document', line: int, column: int, region: Region, namespace: dict
self, document: 'Document', line: int, column: int, region: Region, namespace: Dict[str, Any]
) -> None:
#: The :class:`~sybil.document.Document` from which this example came.
self.document: 'Document' = document
Expand All @@ -51,15 +51,15 @@ def __init__(
self.parsed: Any = region.parsed
#: The :attr:`~sybil.Document.namespace` of the document from
#: which this example came.
self.namespace: dict = namespace
self.namespace: Dict[str, Any] = namespace

def __repr__(self) -> str:
return '<Example path={} line={} column={} using {!r}>'.format(
self.path, self.line, self.column, self.region.evaluator
)

def evaluate(self) -> None:
evaluator = self.document.evaluator or self.region.evaluator
evaluator = self.document.evaluator if self.document.evaluator is not None else self.region.evaluator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think mypy doesn't like checking truthiness of a function:

sybil/example.py:62: error: Function "evaluator" could always be true in boolean context  [truthy-function]

Copy link
Member

Choose a reason for hiding this comment

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

Function "evaluator" could always be true in boolean context is a pretty crappy message :-(
Do you have a link to the mypy docs on what it's sad about here?

The change is needlessly wordy and makes me sad about appeasing mypy's inadequacies :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error was related to https://mypy.readthedocs.io/en/stable/error_code_list.html#check-that-function-isn-t-used-in-boolean-context-truthy-function, because self.document.evaluator was typed as an Evaluator.
I have changed it to be correctly typed as an Optional[Evaluator], and therefore been able to revert the change which made you sad.

That said, we will hit into related problems with self.document.evaluator later, as mypy does not allow assigning to a method:

from typing import Callable, Optional

class Example:
    def __init__(self) -> None:
        # mypy gives "error: Cannot assign to a method  [method-assign]" for this.
        self.foo: Optional[Callable[[], None]] = None

    def foo(self) -> None:
        pass

I think that in order to satisfy mypy, we will either have to add type: ignore s (which I try to avoid, where possible), or change document to be more complex in some ways (likely having another variable to track whether the evaluator should be used).

Copy link
Member

@cjw296 cjw296 Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That "can't assign to a method" feels like a straight up bug in mypy, no?
I'm guessing this problem has come up with other folks attempting to add type hints to existing code of this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That "can't assign to a method" feels like a straight up in mypy, no?

I'm not sure what that means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many discussions on this - with many people wanting mypy to support assigning to a method. See python/mypy#708 for an example.

I do not get the impression that very soon there will me a mypy change to accommodate. You can even see an issue raised by me in 2018 when I wanted to assign to a method precisely as described in the Python documentation: python/mypy#5062 (comment).

In a follow-up PR, I think we will have to decide either to change document, to add a type: ignore, or to ignore all method-assign errors.

Copy link
Member

@cjw296 cjw296 Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can this check be disabled in mypy's config file? Failing that, how many type: ignore comments are we talking about? :sigh:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this check be disabled in mypy's config file?

Yes, that's what I meant when I said: "or to ignore all method-assign errors.".

Any specific mypy error code can be ignored in mypy's config file.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's do that... Might be worth getting a branch going that has mypy turned on and rebase that onto master as more PRs land.

(ie: just the CI, so we know where we're at and what config we'll be using...)

That said, how hard would it be to use pyright instead of mypy? I think @hynek has had some success with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjw296 I'm happy to do those tasks. I think they're best as follow-ups, as this change doesn't yet hit the problem:

  • Add mypy to CI, with pypyoject.toml level ignores for files which are not yet compliant
  • Address the method-assign error. From this conversation, most likely by globally ignoring that error type. I will look into that further in the follow-up, in case there are complications I have not considered here.
  • Look into adding pyright (potentially as a replacement for mypy). I will say that in my own projects, I use both pyright and mypy, and I have found roughly equal numbers of annoyances. I like having both as, despite annoyances, they each have caught a few bugs that the other has not.

result = evaluator(self)
if result:
raise SybilFailure(self, result)