AstarVienna / ScopeSim

A telescope observation simulator for Python.
GNU General Public License v3.0
16 stars 10 forks source link

Enable DeprecationWarnings by default #508

Closed teutoburg closed 3 days ago

teutoburg commented 3 days ago

After some frustration about (intended) warnings that sometimes wouldn't show up, I found out that DeprecationWarnings from anywhere but the __main__ module are ignored by default. This is not what we want when we put those in on purpose, so I changed this here.

codecov[bot] commented 3 days ago

Codecov Report

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

Project coverage is 77.16%. Comparing base (59557a7) to head (30a9cf8). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #508 +/- ## ======================================= Coverage 77.16% 77.16% ======================================= Files 66 66 Lines 8203 8204 +1 ======================================= + Hits 6330 6331 +1 Misses 1873 1873 ```

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


🚨 Try these New Features:

teutoburg commented 3 days ago

Further explanation why this was needed: We have a mechanism to throw a DeprecationWarning when an outdated instrument mode is selected in UserCommands. However, when I tried to see if the warning is actually displayed, it only showed up when changing the mode afterwards, not when setting it initially, even though the code did end up going into the correct if-clause. With this, it now works as it should.

hugobuddel commented 3 days ago

I don´t really understand what is going on, so I trust you.

Why do we even ignore warnings in the first place? They can be useful right? But we apparently always did this, so whatever. It's not that we don't already drown in useless output.

But it seems that if we want users to see the warnings, that we should use FutureWarning, from https://docs.python.org/3/library/warnings.html#warning-categories :

Changed in version 3.7: Previously DeprecationWarning and FutureWarning were distinguished based on whether a feature was being removed entirely or changing its behaviour. They are now distinguished based on their intended audience and the way they’re handled by the default warnings filters.

However, I have the philosophy that there is no real line between a user and a developer. We should encourage everyone to take a look at the code and modify things.

hugobuddel commented 3 days ago

I've worked with this code base for years, and there are still many places that I have never seen.. Maybe we should have a reading club or something like that :books:

teutoburg commented 3 days ago

But it seems that if we want users to see the warnings, that we should use FutureWarning, from https://docs.python.org/3/library/warnings.html#warning-categories :

Huh, I'm sure I read that once but totally forgot that the FutureWarning exists. So I suppose ScopeSim (the application) should use FutureWarnings and scopesim-core should use DeprecationWarnings? And then there's also the PendingDeprecationWarning which I think I've used once or twice somewhere, but probably not correctly either. Ehh, let's just use DepreciationWarnings for things that are (or will be) deprecated, seems good enough...

teutoburg commented 3 days ago

I'm also uncertain when to use RuntimeWarning vs. UserWarning. Or should we perhaps make our own ScopeSimWarning (like Astropy has)? And what about logger.warning vs. warnings.warn? There should be one and preferably only one obviously correct way to do this 🙃