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

Type hint sybil/example.py #78

Merged
merged 2 commits into from
Sep 9, 2023
Merged

Conversation

adamtheturtle
Copy link
Contributor

This makes mypy --strict sybil/example.py pass.

This makes `mypy --strict sybil/example.py` pass.
sybil/example.py Outdated

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

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

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.

@hynek
Copy link

hynek commented Sep 9, 2023

I wouldn’t call my experience with pyright “successful”. 😅 I’m trying to make my packages compatible with it, but they’re actively being the opposite of nice towards me and attrs so I do it rather begrudgingly.

@adamtheturtle adamtheturtle deleted the sybil-example branch September 16, 2023 08:58
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.

3 participants