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

mypy: enable option strict_optional for 'pylib' (#3317) #3355

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

davidculley
Copy link
Contributor

Fixed all errors within the directory pylib by simply adding assertions. These can be replaced later on by better solutions. But we can now set the option strict_optional to true for the entire directory pylib.

Why this option should be true: https://mypy.readthedocs.io/en/stable/config_file.html#confval-strict_optional

@davidculley davidculley marked this pull request as draft August 13, 2024 15:36
@davidculley davidculley changed the title chore(mypy): enable option strict_optional for pylib (#3317) mypy: enable option strict_optional for pylib (#3317) Aug 13, 2024
@davidculley
Copy link
Contributor Author

mypy works great now, but the changes broke pytest. Need to fix pytest now.

@davidculley davidculley changed the title mypy: enable option strict_optional for pylib (#3317) mypy: enable option strict_optional for 'pylib' (#3317) Aug 13, 2024
@davidculley davidculley force-pushed the feature/3317-mypy-strict-optional-pylib branch from f842f5b to df33a62 Compare August 13, 2024 17:54
@dae
Copy link
Member

dae commented Aug 15, 2024

diff --git a/pylib/anki/models.py b/pylib/anki/models.py
index 0d7024933..d3dd5c886 100644
--- a/pylib/anki/models.py
+++ b/pylib/anki/models.py
@@ -134,9 +134,7 @@ class ModelManager(DeprecatedNamesMixin):
 
     def current(self, for_deck: bool = True) -> NotetypeDict:
         "Get current model. In new code, prefer col.defaults_for_adding()"
-        model_id = self.col.decks.current().get("mid")
-        assert model_id is not None
-        notetype = self.get(model_id)
+        notetype = self.get(self.col.decks.current().get("mid"))
         if not for_deck or not notetype:
             notetype = self.get(self.col.conf["curModel"])
         if notetype:
@@ -154,7 +152,7 @@ class ModelManager(DeprecatedNamesMixin):
         except NotFoundError:
             return None
 
-    def get(self, id: NotetypeId) -> NotetypeDict | None:
+    def get(self, id: NotetypeId | None) -> NotetypeDict | None:
         "Get model with ID, or None."
         # deal with various legacy input types
         if id is None:
diff --git a/pylib/anki/tags.py b/pylib/anki/tags.py
index af1f2c6f9..ce6335991 100644
--- a/pylib/anki/tags.py
+++ b/pylib/anki/tags.py
@@ -121,10 +121,9 @@ class TagManager(DeprecatedNamesMixin):
     def rem_from_str(self, deltags: str, tags: str) -> str:
         "Delete tags if they exist."
 
-        def wildcard(pat: str, repl: str) -> Match:
+        def wildcard(pat: str, repl: str) -> Match | None:
             pat = re.escape(pat).replace("\\*", ".*")
             return_value = re.match(f"^{pat}$", repl, re.IGNORECASE)
-            assert return_value is not None
             return return_value
 
         current_tags = self.split(tags)

@davidculley davidculley deleted the feature/3317-mypy-strict-optional-pylib branch August 15, 2024 12:57
@davidculley davidculley restored the feature/3317-mypy-strict-optional-pylib branch August 15, 2024 13:00
@davidculley davidculley reopened this Aug 15, 2024
@davidculley
Copy link
Contributor Author

davidculley commented Aug 15, 2024

@dae Thanks a lot for the super helpful fix. 🙂

Do these error messages make sense to you?

out/qt/_aqt/forms/widgets_qt6.py:194: error: Item "None" of "Optional[QTreeWidgetItem]" has no attribute "setText"  [union-attr]
out/qt/_aqt/forms/customstudy_qt6.py:111: error: Item "None" of "Optional[QListWidgetItem]" has no attribute "setText"  [union-attr]
out/qt/_aqt/forms/customstudy_qt6.py:113: error: Item "None" of "Optional[QListWidgetItem]" has no attribute "setText"  [union-attr]
out/qt/_aqt/forms/customstudy_qt6.py:115: error: Item "None" of "Optional[QListWidgetItem]" has no attribute "setText"  [union-attr]
out/qt/_aqt/forms/customstudy_qt6.py:117: error: Item "None" of "Optional[QListWidgetItem]" has no attribute "setText"  [union-attr]
out/qt/_aqt/forms/browser_qt6.py:77: error: Item "None" of "Optional[QHeaderView]" has no attribute "setCascadingSectionResizes"  [union-attr]
out/qt/_aqt/forms/browser_qt6.py:78: error: Item "None" of "Optional[QHeaderView]" has no attribute "setHighlightSections"  [union-attr]
out/qt/_aqt/forms/browser_qt6.py:79: error: Item "None" of "Optional[QHeaderView]" has no attribute "setMinimumSectionSize"  [union-attr]
out/qt/_aqt/forms/browser_qt6.py:80: error: Item "None" of "Optional[QHeaderView]" has no attribute "setSortIndicatorShown"  [union-attr]

It seems from aqt.rs that when the following three files are translated to Python code, there's no assert that … is not None or other handling of the case if … is None inserted.

  • qt/aqt/forms/widgets.ui
  • qt/aqt/forms/customstudy.ui
  • qt/aqt/forms/browser.ui

@davidculley davidculley marked this pull request as ready for review August 15, 2024 18:57
@dae
Copy link
Member

dae commented Aug 17, 2024

Making the code generator insert them would be tricky. I suggest

diff --git a/.mypy.ini b/.mypy.ini
index 96dae8669..f15a768c2 100644
--- a/.mypy.ini
+++ b/.mypy.ini
@@ -26,6 +26,8 @@ disallow_untyped_defs = True
 strict_optional = true
 [mypy-aqt.*]
 strict_optional = false
+[mypy-_aqt.*]
+strict_optional = false
 [mypy-anki.importing.*]
 disallow_untyped_defs = False
 [mypy-anki.exporting]

@dae
Copy link
Member

dae commented Aug 17, 2024

Please note that Anki currently fails to start, because you've added an assert in a location where the value can be none. We'll need to do a quick pass over each of the asserts that have been added to double-check the values should never be None, as there may be other instances where the types were not correctly indicating that the value was nullable.

@davidculley
Copy link
Contributor Author

davidculley commented Aug 17, 2024

I suggest

Good idea. Thanks for helping me out. 👍

We'll need to do a quick pass over each of the asserts that have been added to double-check the values should never be None

Absolutely. I have no idea if any of the assertions I added are actually correct. I don't understand the codebase well enough to know that. My plan was to first (with the least amount of effort) make all mypy errors disappear, then to make the pipeline succeed if necessary, and then go over all assertions I added and see if they're actually correct. If any assertion is wrong, replace it with a real solution and repeat the step of making the pipeline succeed. Kind of like an "assertion-driven development". 😅

So with your help, I got the pipeline to succeed now. 🙂 Now we can look which assertions need to be replaced.

@davidculley davidculley marked this pull request as draft August 17, 2024 08:16
@davidculley davidculley force-pushed the feature/3317-mypy-strict-optional-pylib branch from 7c2c190 to 2017bb8 Compare August 17, 2024 15:30
@davidculley davidculley force-pushed the feature/3317-mypy-strict-optional-pylib branch from 2017bb8 to ff4bfcb Compare August 17, 2024 15:38
@davidculley davidculley marked this pull request as ready for review August 17, 2024 15:44
@davidculley
Copy link
Contributor Author

To make the code changes easier to review for you, I moved some changes into their separate pull request (#3364).

The assertions become easier to review if you first merge #3364. 🙂

@dae
Copy link
Member

dae commented Aug 29, 2024

I'd appreciate it if you could make an attempt to review these yourself first - Anki wouldn't even start until the latest commit I pushed, and when I started reviewing the changes, I quickly encountered changes that don't appear to be correct, such as

    def get_image_for_occlusion(self, path: str | None) -> GetImageForOcclusionResponse:
        assert path is not None

@dae
Copy link
Member

dae commented Oct 2, 2024

Do you plan to return to this? I can keep it around if you're currently busy, as it would be a nice change.

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