-
Notifications
You must be signed in to change notification settings - Fork 2
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
Ruff enable fbt #47
Ruff enable fbt #47
Conversation
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.
I had one concern, and a couple small suggestions. I think this is a good approach though!
tests/test_actions.py
Outdated
with warnings.catch_warnings(): | ||
warnings.simplefilter("error") | ||
Enter.the_secret("") | ||
|
||
with warnings.catch_warnings(): | ||
warnings.simplefilter("error") | ||
Enter("", mask=True) |
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.
I don't think you're actually testing anything here, i think you need to add a as caught_warnings
to the context manager and then do some assertions on it later. See the docs here: https://docs.python.org/3/library/warnings.html#testing-warnings
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.
This same comment applies to the other tests below.
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.
This test works, albeit it's proving a negative. It's testing to ensure the warning does NOT occur when mask
keyword is used.
It wasn't obvious to me either, but using with warnings.catch_warnings()
along with warnings.simplefilter("error")
causes all warnings to raise an exception when they occur.
You can try it out by removing the keyword.
# THIS passes (as expected)
with warnings.catch_warnings():
warnings.simplefilter("error")
Enter("", mask=True)
# but THIS will fail
with warnings.catch_warnings():
warnings.simplefilter("error")
Enter("", True)
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.
OK, i see now that we get an error for the raised warning, but i don't like that the error is the warning, you know? I'd prefer an assertion that makes it clear what the problem was. Is that possible to do with what they've got in the docs?
I guess it'll only be relevant for a couple months, so i think i can be OK with this.
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.
I updated the tests. They explicitly state when a warning was raised unexpectedly, rather than only raising the warning.
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.
Very nice approach! Let's get this in and do a little release soon to get the deprecation out.
tests/test_actions.py
Outdated
with warnings.catch_warnings(): | ||
warnings.simplefilter("error") | ||
Enter.the_secret("") | ||
|
||
with warnings.catch_warnings(): | ||
warnings.simplefilter("error") | ||
Enter("", mask=True) |
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.
OK, i see now that we get an error for the raised warning, but i don't like that the error is the warning, you know? I'd prefer an assertion that makes it clear what the problem was. Is that possible to do with what they've got in the docs?
I guess it'll only be relevant for a couple months, so i think i can be OK with this.
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.
Actually we probably want to add a page to the docs about deprecations, like this one in ScreenPy: https://screenpy-docs.readthedocs.io/en/latest/deprecations.html
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 all the updates! I appreciate the added effort to deprecate before removal. I'm sure there's someone out there who is using these Actions this way.
The final ruff rule.
FBT triggered on the fact that actions like
Enter
should not use bool in a positional argument. Unfortunately changing the constructor for these actions could cause folks some issues. This PR attempts to give users some time to adjust before we update these constructors.