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

fix: squiggly lines begone #399

Merged
merged 9 commits into from
Mar 7, 2024
Merged

fix: squiggly lines begone #399

merged 9 commits into from
Mar 7, 2024

Conversation

jmbuhr
Copy link
Collaborator

@jmbuhr jmbuhr commented Mar 1, 2024

I don't like red squiggly lines, so this PR fixes all typing errors and properly tells pyright to chill in places where we know for certain that a type mismatch is expected. This ensures that we don't get used to pyright warnings and as such they actually stay relevant. If you see a red squiggly line after this, it's probably something that should be addressed.

A slightly bigger change included here is that the BondOperation and Place recipe steps are now no longer dataclasses, but rather regular classes. With the way we handle custom init, setters and getters in those, this makes it a lot clearer to the reader (and the type checker... begone red squiggly lines!) to simply implement the little things we would get from a dataclass (init, eq, hash, repr and str dunder methods) instead of patching on top with post_init as we previously did.

Imports have been sorted with isort.

After this, the only hints you should see from pyright are:

  • Code is unreachable for theses special cases
if sys.version_info > (3, 10):
    from importlib_metadata import version
else:
    from importlib.metadata import version
  • is not accessed for
# needed for eval of type_scheme from schema
# don't remove even if lsp says it's unused
import pathlib
from pathlib import Path
  • is not accessed for pytest fixtures as test function parameters
  • errors in tests with with pytest.raises..., obviously

@jmbuhr jmbuhr changed the title minor cleanup and some fixes fix: squiggly lines begone Mar 6, 2024
@jmbuhr jmbuhr added the testthis label Mar 6, 2024
@jmbuhr jmbuhr requested a review from KRiedmiller March 6, 2024 15:51
@KRiedmiller
Copy link
Collaborator

KRiedmiller commented Mar 6, 2024

In recipes, I think we tried already in the past to change away from dataclasses, but finally did not, as dataclasses provide proper comparison functions based on the contents. Your implementations of those look fine, but is a lot more code now and in the future to maintain. Also, it is more error prone as one need to change one thing in multiple places.
Whats the issue with these dataclasses?
On the other hand, I don't expect regular changes here, so as long as it works....

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Mar 6, 2024

I'd rather have the code for comparisons there explicitly, rather than derived by the dataclass because the derivation may not actually do what we expect it to do. The issue was that with the custom setters and getters plus the post init method we were already working against the dataclass rather than with it, so instead of being of frankenstein dataclass (that the typechecker hates) I'd rather have it be a regular class.

@KRiedmiller
Copy link
Collaborator

We only use officially supported functionality by Python, while type checkers are not first-party Python, and apparently can't keep up with the development.
I felt the dataclasses were quite elegant, and besides the post_init, there was not really anything unusual. And even there, we still need the same code now.

Anyway, I think we have enough tests already for this implemented that it will not break unnoticed in the future I think, so it's fine by me.

@jmbuhr jmbuhr merged commit b1210e4 into main Mar 7, 2024
@jmbuhr jmbuhr deleted the refactor/minor-typing-cleanup branch March 7, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants