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

Ruff enable fbt #47

Merged
merged 10 commits into from
Feb 14, 2024
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ select = [
"ERA", # eradicate
"F", # Pyflakes
"FA", # flake8-future-annotations
# "FBT", # flake8-boolean-trap
"FBT", # flake8-boolean-trap
"FIX", # flake8-fixme
"FLY", # flynt
"I", # isort
Expand Down
6 changes: 5 additions & 1 deletion screenpy_selenium/actions/enter.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from screenpy.pacing import aside, beat
from selenium.common.exceptions import WebDriverException

from ..common import pos_args_deprecated
from ..speech_tools import KEY_NAMES

if TYPE_CHECKING:
Expand Down Expand Up @@ -152,7 +153,10 @@ def add_to_chain(
for key in self.following_keys:
send_keys(key)

def __init__(self: SelfEnter, text: str, mask: bool = False) -> None:
@pos_args_deprecated("mask")
def __init__(
self: SelfEnter, text: str, mask: bool = False # noqa: FBT001, FBT002
) -> None:
self.text = text
self.target = None
self.following_keys = []
Expand Down
8 changes: 7 additions & 1 deletion screenpy_selenium/actions/hold_down.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from screenpy.pacing import beat
from selenium.webdriver.common.keys import Keys

from ..common import pos_args_deprecated
from ..speech_tools import KEY_NAMES

if TYPE_CHECKING:
Expand Down Expand Up @@ -88,7 +89,12 @@ def add_to_chain(
msg = "HoldDown must be told what to hold down."
raise UnableToAct(msg)

def __init__(self: SelfHoldDown, key: str | None = None, lmb: bool = False) -> None:
@pos_args_deprecated("lmb")
def __init__(
self: SelfHoldDown,
key: str | None = None,
lmb: bool = False, # noqa: FBT001, FBT002
) -> None:
self.key = key
self.lmb = lmb
self.target = None
Expand Down
8 changes: 7 additions & 1 deletion screenpy_selenium/actions/release.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from screenpy.pacing import beat
from selenium.webdriver.common.keys import Keys

from ..common import pos_args_deprecated
from ..speech_tools import KEY_NAMES

if TYPE_CHECKING:
Expand Down Expand Up @@ -75,7 +76,12 @@ def add_to_chain(self: SelfRelease, _: Actor, the_chain: ActionChains) -> None:
msg = "Release must be told what to release."
raise UnableToAct(msg)

def __init__(self: SelfRelease, key: str | None = None, lmb: bool = False) -> None:
@pos_args_deprecated("lmb")
def __init__(
self: SelfRelease,
key: str | None = None,
lmb: bool = False, # noqa: FBT001, FBT002
) -> None:
self.key = key
self.lmb = lmb
self.description = "LEFT MOUSE BUTTON" if lmb else KEY_NAMES[key]
Expand Down
45 changes: 45 additions & 0 deletions screenpy_selenium/common.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
"""module to hold shared objects."""
bandophahita marked this conversation as resolved.
Show resolved Hide resolved

from __future__ import annotations

import warnings
from functools import wraps
from typing import TYPE_CHECKING, Callable, TypeVar

from typing_extensions import ParamSpec

if TYPE_CHECKING:
P = ParamSpec("P")
T = TypeVar("T")
Function = Callable[P, T]


def pos_args_deprecated(*keywords: str) -> Function:
"""Warn users which positional arguments should be called via keyword."""

def deprecated(func: Function) -> Function:
argnames = func.__code__.co_varnames[: func.__code__.co_argcount]
i = min([argnames.index(kw) for kw in keywords])
kw_argnames = argnames[i:]

@wraps(func)
def wrapper(*args: P.args, **kwargs: P.kwargs) -> Function:
# call the function first, to make sure the signature matches
ret_value = func(*args, **kwargs)

args_that_should_be_kw = args[i:]
if args_that_should_be_kw:
posargnames = ", ".join(kw_argnames)

msg = (
f"Warning: positional arguments `{posargnames}` for "
f"`{func.__qualname__}` are deprecated. "
f"Please use keyword arguments instead."
bandophahita marked this conversation as resolved.
Show resolved Hide resolved
)
warnings.warn(msg, DeprecationWarning, stacklevel=2)

return ret_value

return wrapper

return deprecated
7 changes: 6 additions & 1 deletion screenpy_selenium/questions/selected.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
from screenpy.pacing import beat
from selenium.webdriver.support.ui import Select as SeleniumSelect

from ..common import pos_args_deprecated

if TYPE_CHECKING:
from screenpy import Actor

Expand Down Expand Up @@ -90,6 +92,9 @@ def answered_by(self: SelfSelected, the_actor: Actor) -> str | list[str]:
return [e.text for e in select.all_selected_options]
return select.first_selected_option.text

def __init__(self: SelfSelected, target: Target, multi: bool = False) -> None:
@pos_args_deprecated("multi")
def __init__(
self: SelfSelected, target: Target, multi: bool = False # noqa: FBT001, FBT002
) -> None:
self.target = target
self.multi = multi
7 changes: 6 additions & 1 deletion screenpy_selenium/questions/text.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

from screenpy.pacing import beat

from ..common import pos_args_deprecated

if TYPE_CHECKING:
from screenpy import Actor

Expand Down Expand Up @@ -70,6 +72,9 @@ def answered_by(self: SelfText, the_actor: Actor) -> str | list[str]:
return [e.text for e in self.target.all_found_by(the_actor)]
return self.target.found_by(the_actor).text

def __init__(self: SelfText, target: Target, multi: bool = False) -> None:
@pos_args_deprecated("multi")
def __init__(
self: SelfText, target: Target, multi: bool = False # noqa: FBT001, FBT002
) -> None:
self.target = target
self.multi = multi
40 changes: 40 additions & 0 deletions tests/test_actions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import warnings
from typing import cast
from unittest import mock

Expand Down Expand Up @@ -435,6 +436,19 @@ def new_method(self) -> bool:

assert SubEnter.the_text("blah").new_method() is True

def test_positional_arg_warns(self) -> None:
with pytest.warns(DeprecationWarning):
Enter("", True)

def test_keyword_arg_does_not_warn(self) -> None:
with warnings.catch_warnings():
warnings.simplefilter("error")
Enter.the_secret("")

with warnings.catch_warnings():
warnings.simplefilter("error")
Enter("", mask=True)
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

@bandophahita bandophahita Feb 8, 2024

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)

Copy link
Member

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.

Copy link
Contributor Author

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.



class TestEnter2FAToken:
def test_can_be_instantiated(self) -> None:
Expand Down Expand Up @@ -619,6 +633,19 @@ def new_method(self) -> bool:

assert SubHoldDown.left_mouse_button().new_method() is True

def test_positional_arg_warns(self) -> None:
with pytest.warns(DeprecationWarning):
HoldDown(Keys.LEFT_ALT, True)

def test_keyword_arg_does_not_warn(self) -> None:
with warnings.catch_warnings():
warnings.simplefilter("error")
HoldDown.left_mouse_button()

with warnings.catch_warnings():
warnings.simplefilter("error")
HoldDown(lmb=True)


class TestMoveMouse:
def test_can_be_instantiated(self) -> None:
Expand Down Expand Up @@ -884,6 +911,19 @@ def new_method(self) -> bool:

assert SubRelease.left_mouse_button().new_method() is True

def test_positional_arg_warns(self) -> None:
with pytest.warns(DeprecationWarning):
Release(Keys.LEFT_ALT, True)

def test_keyword_arg_does_not_warn(self) -> None:
with warnings.catch_warnings():
warnings.simplefilter("error")
Release.left_mouse_button()

with warnings.catch_warnings():
warnings.simplefilter("error")
Release(lmb=True)


class TestRespondToThePrompt:
def test_can_be_instantiated(self) -> None:
Expand Down
27 changes: 27 additions & 0 deletions tests/test_questions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import warnings
from unittest import mock

import pytest
Expand Down Expand Up @@ -309,6 +310,19 @@ def test_describe(self) -> None:
Selected(TARGET).describe() == f"The selected option(s) from the {TARGET}."
)

def test_positional_arg_warns(self) -> None:
with pytest.warns(DeprecationWarning):
Selected(TARGET, True)

def test_keyword_arg_does_not_warn(self) -> None:
with warnings.catch_warnings():
warnings.simplefilter("error")
Selected.options_from_the(TARGET)

with warnings.catch_warnings():
warnings.simplefilter("error")
Selected(TARGET, multi=True)


class TestText:
def test_can_be_instantiated(self) -> None:
Expand Down Expand Up @@ -379,3 +393,16 @@ def test_ask_for_text_of_the_alert(self, Tester: Actor) -> None:

def test_describe(self) -> None:
assert TextOfTheAlert().describe() == "The text of the alert."

def test_positional_arg_warns(self) -> None:
with pytest.warns(DeprecationWarning):
Text(TARGET, True)

def test_keyword_arg_does_not_warn(self) -> None:
with warnings.catch_warnings():
warnings.simplefilter("error")
Text.of_all(TARGET)

with warnings.catch_warnings():
warnings.simplefilter("error")
Text(TARGET, multi=True)
Loading