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 C linting rules #138

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Enable C linting rules #138

merged 4 commits into from
Jun 26, 2024

Conversation

ddundo
Copy link
Member

@ddundo ddundo commented Jun 25, 2024

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 ddundo self-assigned this Jun 25, 2024
@ddundo ddundo added bug Something isn't working clarity Something isn't sufficiently clear labels Jun 25, 2024
@ddundo ddundo added this to the Version 1 milestone Jun 25, 2024
@ddundo ddundo requested a review from jwallwork23 June 25, 2024 18:26
Copy link
Member

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

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.

animate/interpolation.py Outdated Show resolved Hide resolved
animate/interpolation.py Outdated Show resolved Hide resolved
animate/utility.py Outdated Show resolved Hide resolved
@ddundo
Copy link
Member Author

ddundo commented Jun 26, 2024

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
Copy link
Member

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 ddundo requested a review from jwallwork23 June 26, 2024 12:21
@ddundo
Copy link
Member Author

ddundo commented Jun 26, 2024

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

@ddundo ddundo merged commit 0934ab6 into main Jun 26, 2024
1 check passed
@ddundo ddundo deleted the 108_c_rules branch June 26, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working clarity Something isn't sufficiently clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix B and C linting rules
2 participants