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 DeprecationWarnings by default #508

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Enable DeprecationWarnings by default #508

merged 1 commit into from
Nov 19, 2024

Conversation

teutoburg
Copy link
Contributor

@teutoburg teutoburg commented Nov 19, 2024

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.

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.
@teutoburg teutoburg added the enhancement New feature or request label Nov 19, 2024
@teutoburg teutoburg self-assigned this Nov 19, 2024
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teutoburg teutoburg marked this pull request as ready for review November 19, 2024 16:08
@teutoburg teutoburg requested a review from hugobuddel November 19, 2024 16:08
@teutoburg
Copy link
Contributor Author

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

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

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 📚

@teutoburg
Copy link
Contributor Author

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
Copy link
Contributor Author

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 🙃

@teutoburg teutoburg merged commit 86c3e1f into main Nov 19, 2024
22 checks passed
@teutoburg teutoburg deleted the fh/more-warnings branch November 19, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants