search

mesh-adaptation / animate

Anisotropic mesh adaptation toolkit for Firedrake
MIT License
5 stars 0 forks source link

Enable C linting rules #138

Closed ddundo closed 3 months ago

ddundo commented 3 months ago

Closes #108.

After #120, the remaining linting errors are:

animate/interpolation.py:296:5: C901 `clement_interpolant` is too complex (19 > 10)
animate/quality.py:91:18: C408 Unnecessary `dict` call (rewrite as a literal)
animate/utility.py:97:5: C901 `norm` is too complex (13 > 10)
animate/utility.py:169:5: C901 `errornorm` is too complex (12 > 10)

This PR addresses all of them apart from the final one, to which I added noqa: C901. The complexity mostly comes from the if isinstance checks so I don't think it's that bad to begin with. And I guess I could replace those with assert but I would rather not do that. @jwallwork23 do you see a nice way to simplify it please? :)

ddundo commented 3 months ago

Thanks for this @ddundo. This linter option seems a bit over-the-top to me, to be honest. But I have a few suggestions that could improve things a bit.

Thanks @jwallwork23 - very good points! And before I address them, would you prefer if I added C901 to the ignore list if you think it's over-the-top? Then I could revert back to how it was if you'd like

jwallwork23 commented 3 months ago

Thanks @jwallwork23 - very good points! And before I address them, would you prefer if I added C901 to the ignore list if you think it's over-the-top? Then I could revert back to how it was if you'd like

No it's okay, let's stick with it. It's better to go with linter recommendations rather than ignoring them.

ddundo commented 3 months ago

Thanks @jwallwork23 - this tidies it up nicely :) ready for review again