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

Enable strict_optional for aqt/editor.py #3500

Merged
merged 7 commits into from
Oct 15, 2024

Conversation

bpnguyen107
Copy link
Contributor

There are a little under 600 mypy errors in 48 files left in the aqt directory, so I'm going to try to work through them file by file.

Comment on lines -245 to +273
if func:

def wrapped_func(editor: Editor) -> None:
self.call_after_note_saved(
functools.partial(func, editor), keepFocus=True
)
def wrapped_func(editor: Editor) -> None:
self.call_after_note_saved(functools.partial(func, editor), keepFocus=True)

self._links[cmd] = wrapped_func
self._links[cmd] = wrapped_func

if keys:
if keys:

def on_activated() -> None:
wrapped_func(self)
def on_activated() -> None:
wrapped_func(self)

if toggleable:
# generate a random id for triggering toggle
id = id or str(randrange(1_000_000))
if toggleable:
# generate a random id for triggering toggle
id = id or str(randrange(1_000_000))

def on_hotkey() -> None:
on_activated()
self.web.eval(
f'toggleEditorButton(document.getElementById("{id}"));'
)
def on_hotkey() -> None:
on_activated()
self.web.eval(
f'toggleEditorButton(document.getElementById("{id}"));'
)

else:
on_hotkey = on_activated
else:
on_hotkey = on_activated

QShortcut( # type: ignore
QKeySequence(keys),
self.widget,
activated=on_hotkey,
)
QShortcut( # type: ignore
QKeySequence(keys),
self.widget,
activated=on_hotkey,
)
Copy link
Contributor Author

@bpnguyen107 bpnguyen107 Oct 15, 2024

Choose a reason for hiding this comment

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

I moved this code out of the if-block because the mypy error was qt/aqt/editor.py:245: error: Function "func" could always be true in boolean context [truthy-function]

An alternative to this fix would be changing the type in the function definition to func: Callable[[Editor], None] | None

Copy link
Member

Choose a reason for hiding this comment

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

I'd make the same call 👍

@@ -696,7 +704,7 @@ def cleanup(self) -> None:
# prevent any remaining evalWithCallback() events from firing after C++ object deleted
if self.web:
self.web.cleanup()
self.web = None
self.web = None # type: ignore
Copy link
Contributor Author

@bpnguyen107 bpnguyen107 Oct 15, 2024

Choose a reason for hiding this comment

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

I did # type: ignore here instead of changing self.web's type to EditorWebView | None so that I didn't have to put a bunch of assert self.web is not None's throughout the file. However, I can do the latter if that's more desirable.

Copy link
Member

Choose a reason for hiding this comment

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

In cases like this where the property ends up getting set as part of init() and unset only on cleanup, it's not very risky, and it avoids the extra assertion boilerplate. 👍

Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

Thanks Ben, this all looks good.

Comment on lines -245 to +273
if func:

def wrapped_func(editor: Editor) -> None:
self.call_after_note_saved(
functools.partial(func, editor), keepFocus=True
)
def wrapped_func(editor: Editor) -> None:
self.call_after_note_saved(functools.partial(func, editor), keepFocus=True)

self._links[cmd] = wrapped_func
self._links[cmd] = wrapped_func

if keys:
if keys:

def on_activated() -> None:
wrapped_func(self)
def on_activated() -> None:
wrapped_func(self)

if toggleable:
# generate a random id for triggering toggle
id = id or str(randrange(1_000_000))
if toggleable:
# generate a random id for triggering toggle
id = id or str(randrange(1_000_000))

def on_hotkey() -> None:
on_activated()
self.web.eval(
f'toggleEditorButton(document.getElementById("{id}"));'
)
def on_hotkey() -> None:
on_activated()
self.web.eval(
f'toggleEditorButton(document.getElementById("{id}"));'
)

else:
on_hotkey = on_activated
else:
on_hotkey = on_activated

QShortcut( # type: ignore
QKeySequence(keys),
self.widget,
activated=on_hotkey,
)
QShortcut( # type: ignore
QKeySequence(keys),
self.widget,
activated=on_hotkey,
)
Copy link
Member

Choose a reason for hiding this comment

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

I'd make the same call 👍

@@ -696,7 +704,7 @@ def cleanup(self) -> None:
# prevent any remaining evalWithCallback() events from firing after C++ object deleted
if self.web:
self.web.cleanup()
self.web = None
self.web = None # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

In cases like this where the property ends up getting set as part of init() and unset only on cleanup, it's not very risky, and it avoids the extra assertion boilerplate. 👍

@dae dae merged commit a537d34 into ankitects:main Oct 15, 2024
1 check passed
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