Skip to content

Refactor conv warnings #2755

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

Merged
merged 13 commits into from
Nov 6, 2021
Merged

Refactor conv warnings #2755

merged 13 commits into from
Nov 6, 2021

Conversation

Poolitzer
Copy link
Member

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Added myself alphabetically to AUTHORS.rst (optional)

This PR closes #2615.

I did make a decision while implementing this: I don't think it makes sense to break the for loops after one warning. The warnings menu will not show more then one unique warning anyway, and we swallow non unique ones this way or unique ones if the user changes their filterwarning method.

also added a link to the _per* FAQ page
also improved one warning message and removed a break
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR :) I've left a couple of nitpicks. In addition to that, could you add an item to the PR template about adding new handlers to these checks?

Poolitzer and others added 2 commits November 3, 2021 12:21
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
also added a checkmark to the PR template
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of tiny changes-

@harshil21 harshil21 added this to the v14 milestone Nov 3, 2021
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates :) just a bit more nitpicking …

Poolitzer and others added 3 commits November 4, 2021 14:23
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice addition 👍 if @harshil21 gives a LGTM as well, we can merge :)

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some more optimizations-

also added test for the nested conv warn
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@Bibo-Joshi
Copy link
Member

Tests failing due to API changes. Merging :)

@Bibo-Joshi Bibo-Joshi merged commit 3e8d1fa into v14 Nov 6, 2021
@Bibo-Joshi Bibo-Joshi deleted the refactor_conv_warnings branch November 6, 2021 14:19
@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2021
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔌 enhancement pr description: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Issue an warning if PollHandler (etc.?) is added into a conv handler
3 participants