Skip to content

Go back to LBYL programming style (instead of EAFP) and several mypy fixes in fem.petsc and la.petsc #3790

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

Merged
merged 25 commits into from
Jul 19, 2025

Conversation

francesco-ballarin
Copy link
Member

@francesco-ballarin francesco-ballarin commented Jul 9, 2025

(replaces #3789 which was not branched off of main)

@francesco-ballarin francesco-ballarin added this to the 0.10.0 milestone Jul 9, 2025
@schnellerhase
Copy link
Contributor

schnellerhase commented Jul 9, 2025

I think there are quite a few more cases of the EAFP paradigm. In mesh.py:637-666, bcs.py:51-55, bcs.py:87-91, bcs.py:225-234, forms.py:358-362, function.py:144-147, function.py:475-484, function.py:602-613, function.py:628-631, function.py:683-690, fem/petsc.py (handled in #3785), io/utils.py:84-90, io/utils.py:97-104.

If not addressed here, we should keep the issue open.

@francesco-ballarin
Copy link
Member Author

If not addressed here, we should keep the issue open.

I edited the PR description removing "Fixes".

@francesco-ballarin francesco-ballarin changed the title Go back to LBYL programming style (instead of EAFP) in fem.petsc and la.petsc Go back to LBYL programming style (instead of EAFP) and several mypy fixes in fem.petsc and la.petsc Jul 11, 2025
@francesco-ballarin
Copy link
Member Author

francesco-ballarin commented Jul 11, 2025

I now removed # mypy: ignore-errors, and the scope of the PR changed massively. Most of the changes are now related to mypy fixes which, as @jhale suspected, was basically untested due to the module level ignore.

I also moved from Iterable to Sequence in fem.petsc and la.petsc for all the reasons listed above.

I suggest giving the review of this PR a priority higher than #3787 #3794 . After this one is in, the other two PRs will finish up on fixing #3738, which was the original goal of this PR.

Copy link
Contributor

@schnellerhase schnellerhase left a comment

Choose a reason for hiding this comment

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

We should maybe aim to activate warn_unused_ignores in the mypy settings to get notified when all these PETSc ignores are no longer necessary.

@francesco-ballarin
Copy link
Member Author

We should maybe aim to activate warn_unused_ignores in the mypy settings to get notified when all these PETSc ignores are no longer necessary.

Agreed, but this will not happen in this PR.

@francesco-ballarin francesco-ballarin added this pull request to the merge queue Jul 19, 2025
@francesco-ballarin
Copy link
Member Author

francesco-ballarin commented Jul 19, 2025

Principle agreed upon in previous discussions. Queueing this to merge to unblock other PRs (e.g. #3794).

Merged via the queue into main with commit 350fabe Jul 19, 2025
30 checks passed
@francesco-ballarin francesco-ballarin deleted the francesco/isinstance-2 branch July 19, 2025 06:16
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