Skip to content

Conversation

aaronzo
Copy link

@aaronzo aaronzo commented Oct 16, 2024

workaround for #11933, but also an improved functionality in its own right. The user may set the keyword argument keep_ignores for pytest.warns to avoid catching warnings which were filtered out, in pytest configuration or otherwise. Relevant issue comment

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Add text like closes #XYZW to the PR description and/or commits (where XYZW is the issue number). See the github docs for more information.
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Add yourself to AUTHORS in alphabetical order.

@reaganjlee
Copy link
Contributor

reaganjlee commented Oct 18, 2024

Not a maintainer, but thank you for taking this up!

I think the intuition of the method would be that it already respects the ignore filters, and don't see how a user would not want it that way already, so I don't think an additional argument is needed?

This does address your backward compatibility point thought. I think if this hasn't been as big of an issue to this point then I think it's fine to just make the respect-ignore-filters change without adding an additional argument. I could be wrong though, so open to thoughts from others as well!

@aaronzo
Copy link
Author

aaronzo commented Oct 21, 2024

@reaganjlee I'd be nervous about introducing a breaking change without at least a deprecation cycle - it's going to be hard to assess how often the current behaviour is specifically used. Test suite code is often hacky, so I wouldn't be surprised if changing the default behaviour broke a load of code

@nicoddemus
Copy link
Member

Hi folks

Thanks @aaronzo for the PR!

I'm for now -0 on the functionality -- not sure the extra complexity is worth it for pytest.warns, but if others feel different I won't oppose merging this.

I'd be nervous about introducing a breaking change without at least a deprecation cycle - it's going to be hard to assess how often the current behaviour is specifically used. Test suite code is often hacky, so I wouldn't be surprised if changing the default behaviour broke a load of code

100% agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants