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

Avoid warnings when accessing attributes on classes that may not be regular properties #134

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Aug 19, 2021

In astropy core, we need to do horrendous things like:

https://github.com/astropy/astropy/blob/46ca59e9a0bfa3b6687411ea7a6364fdebf91914/astropy/visualization/wcsaxes/patches.py#L17-L26

because sometimes Matplotlib deprecate properties - however because those are custom properties and not regular properties, we make use of getattr in sphinx-automodapi to access these which triggers the code inside the properties to run. So this PR just ignores any warning that may occur from running those properties which I think is sensible since there's nothing we can do about those warnings (this will be useful regardless of what dependency deprecates attributes/properties)

The test I added still fails though as the warning still appears a few times (the fix I did takes it down from 7 to 4 warnings). If anyone else fancies figuring out how to fix the test that would be great! Otherwise I'll try and pick this up when I have some time again (but probably not for another month :-/)

@astrofrog
Copy link
Member Author

I think I've figured it out. Now the question is, do we want to blankly silence all warnings? Or do we want to potentially make this opt-in behavior and ask the user to specify warnings that are safe to ignore? I'm trying to think whether there are legitimate cases where we might not want to silence the warnings, but can't think of any at the moment.

@bsipocz
Copy link
Member

bsipocz commented Aug 19, 2021

It may not be the case for these warnings, but it happened a few times in general to notify upstream for issues when they didn't had their CI trigger on certain warnings. So if it doesn't add too much complication I would vote for an opt-in solution, in case we ever need to turn it off for debug purposes.

@bsipocz
Copy link
Member

bsipocz commented Apr 13, 2023

@astrofrog - do you still need this? If yes, would you mind rebase and get it through the finish line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants