-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Refactor conv warnings #2755
Conversation
also added a link to the _per* FAQ page
also improved one warning message and removed a break
There was a problem hiding this 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?
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
also added a checkmark to the PR template
There was a problem hiding this 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-
There was a problem hiding this 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 …
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com> Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
There was a problem hiding this 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 :)
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Tests failing due to API changes. Merging :) |
Checklist for PRs
.. versionadded:: version
,.. versionchanged:: version
or.. deprecated:: version
to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)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.