pybamm-team / PyBaMM

Fast and flexible physics-based battery models in Python
https://www.pybamm.org/
BSD 3-Clause "New" or "Revised" License
876 stars 492 forks source link

Removing false flagging of assert statements from tests. #4236

Open prady0t opened 2 days ago

prady0t commented 2 days ago

Codacy keeps flagging assert statements from tests. This added rule solves this.

codecov[bot] commented 2 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.54%. Comparing base (6a0cd9a) to head (4048db0).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #4236 +/- ## ======================================== Coverage 99.54% 99.54% ======================================== Files 288 288 Lines 21886 21886 ======================================== Hits 21786 21786 Misses 100 100 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

agriyakhetarpal commented 2 days ago

Doesn't this ignore all asserts? We do not want asserts in the regular codebase.

Yes, it does and we discussed this – I don't think there is a way to configure it per path, and many other projects have the same issue: https://github.com/fossasia/query-server/issues/332. We can look at assert statements in PRs manually though, even though that means having more work on our end.

kratman commented 2 days ago

Is codacy even doing anything for us? Can we just make ruff stricter?

agriyakhetarpal commented 2 days ago

@valentinsulzer brought up the idea, and yes, we can do it because Ruff has the same rules available: https://docs.astral.sh/ruff/rules/#flake8-bandit-s

kratman commented 2 days ago

I do think it is good to check for complexity (which codacy does), and asserts should be avoided in the main codebase. I think there is a lot we could improve by ramping up the ruff strictness. I think it should probably get a lot stricter based on the warnings I see in PyCharm