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

#19: Fix typing for aliases. #20

Merged
merged 6 commits into from
May 13, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions screenpy/actions/attach_the_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Attach a file.
"""

import os
from typing import Any

from screenpy import Actor
Expand All @@ -23,11 +24,16 @@ class AttachTheFile:
)
"""

def describe(self) -> str:
"""Describe the Action in present tense."""
return f"Attach a file named {self.filename}."

# no beat, to make reading reports easier.
def perform_as(self, _: Actor) -> None:
"""Direct the Narrator to attach a file."""
the_narrator.attaches_a_file(self.path, **self.attach_kwargs)
the_narrator.attaches_a_file(self.filepath, **self.attach_kwargs)

def __init__(self, path: str, **kwargs: Any) -> None:
self.path = path
def __init__(self, filepath: str, **kwargs: Any) -> None:
self.filepath = filepath
self.filename = os.path.basename(filepath)
self.attach_kwargs = kwargs
35 changes: 31 additions & 4 deletions screenpy/actions/eventually.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,44 @@ def perform_as(self, the_actor: "Actor") -> None:
the_actor.attempts_to(self.eventually)

def for_(self, amount: float) -> _TimeframeBuilder:
"""Set for how long the actor should continue trying."""
"""Set for how long the actor should continue trying.

Aliases:
* :meth:`~screenpy.actions.Eventually.trying_for_no_longer_than`
* :meth:`~screenpy.actions.Eventually.trying_for`
* :meth:`~screenpy.actions.Eventually.waiting_for`
"""
return self._TimeframeBuilder(self, amount, "timeout")

trying_for_no_longer_than = trying_for = waiting_for = for_
def trying_for_no_longer_than(self, amount: float) -> _TimeframeBuilder:
"""Alias for :meth:`~screenpy.actions.Eventually.for_`."""
return self.for_(amount)

def trying_for(self, amount: float) -> _TimeframeBuilder:
"""Alias for :meth:`~screenpy.actions.Eventually.for_`."""
return self.for_(amount)

def waiting_for(self, amount: float) -> _TimeframeBuilder:
"""Alias for :meth:`~screenpy.actions.Eventually.for_`."""
return self.for_(amount)

def polling(self, amount: float) -> _TimeframeBuilder:
"""Adjust the polling frequency."""
"""Adjust the polling frequency.

Aliases:
* :meth:`~screenpy.actions.Eventually.polling_every`
* :meth:`~screenpy.actions.Eventually.trying_every`
"""
self.poll = amount
return self._TimeframeBuilder(self, amount, "poll")

trying_every = polling_every = polling
def polling_every(self, amount: float) -> _TimeframeBuilder:
"""Alias for :meth:`~screenpy.actions.Eventually.polling`."""
return self.for_(amount)

def trying_every(self, amount: float) -> _TimeframeBuilder:
"""Alias for :meth:`~screenpy.actions.Eventually.polling`."""
return self.for_(amount)

def describe(self) -> str:
"""Describe the Action in present tense."""
Expand Down
11 changes: 9 additions & 2 deletions screenpy/actions/make_note.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,17 @@ class MakeNote:

@classmethod
def of(cls, question: Union[Answerable, Any]) -> "MakeNote":
"""Supply the Question to answer and its arguments."""
"""Supply the Question to answer and its arguments.

Aliases:
* :meth:`~screenpy.actions.MakeNote.of_the`
"""
return cls(question)

of_the = of
@classmethod
def of_the(cls, question: Union[Answerable, Any]) -> "MakeNote":
"""Alias for :meth:`~screenpy.actions.MakeNote.of`."""
return cls.of(question)

def as_(self, key: str) -> "MakeNote":
"""Set the key to use to recall this noted value."""
Expand Down
10 changes: 8 additions & 2 deletions screenpy/actions/pause.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,18 @@ def for_(cls, number: float) -> "Pause":
return cls(number)

def seconds_because(self, reason: str) -> "Pause":
"""Use seconds and provide a reason for the pause."""
"""Use seconds and provide a reason for the pause.

Aliases:
* :meth:`~screenpy.actions.Pause.second_because`
"""
self.unit = f"second{'s' if self.number != 1 else ''}"
self.reason = self._massage_reason(reason)
return self

second_because = seconds_because
def second_because(self, reason: str) -> "Pause":
"""Alias for :meth:`~screenpy.actions.Pause.seconds_because`."""
return self.seconds_because(reason)

def milliseconds_because(self, reason: str) -> "Pause":
"""Use milliseconds and provide a reason for the pause."""
Expand Down
79 changes: 69 additions & 10 deletions screenpy/actor.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from .protocols import Forgettable, Performable
from .speech_tools import get_additive_description

# pylint: disable=too-many-public-methods
Copy link
Member Author

@perrygoy perrygoy May 13, 2022

Choose a reason for hiding this comment

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

I'm not sure why pylint was OK with all the aliases when they were created using = but complains now. This sure shut it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

The aliases within _TimeframeBuilder should get converted also.
millisecond = milliseconds

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't convert them because they aren't surfaced for documentation (currently) and also do not have the problems that the classmethods did.

... Unless they do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're right. They are just aliases. AFAICT they don't have the same problem as the classmethods.


ENTRANCE_DIRECTIONS = [
"{actor} appears from behind the backdrop!",
"{actor} arrives on stage!",
Expand Down Expand Up @@ -56,11 +58,17 @@ def named(cls, name: Text) -> "Actor":
return cls(name)

def who_can(self, *abilities: Forgettable) -> "Actor":
"""Add one or more Abilities to this Actor."""
"""Add one or more Abilities to this Actor.

Aliases:
* :meth:`~screenpy.actor.Actor.can`
"""
self.abilities.extend(abilities)
return self

can = who_can
def can(self, *abilities: Forgettable) -> "Actor":
"""Alias for :meth:`~screenpy.actor.Actor.who_can`."""
return self.who_can(*abilities)

def has_cleanup_tasks(self, *tasks: Performable) -> "Actor":
"""Assign one or more tasks to the Actor to perform when exiting."""
Expand All @@ -78,38 +86,62 @@ def has_ordered_cleanup_tasks(self, *tasks: Performable) -> "Actor":
The tasks given to this method must be performed successfully in
order. If any task fails, any subsequent tasks will not be attempted
and will be discarded.

Aliases:
* :meth:`~screenpy.actor.Actor.has_cleanup_task`
* :meth:`~screenpy.actor.Actor.with_cleanup_task`
* :meth:`~screenpy.actor.Actor.with_ordered_cleanup_tasks`
"""
self.ordered_cleanup_tasks.extend(tasks)
return self

has_cleanup_task = has_ordered_cleanup_tasks
with_cleanup_task = with_ordered_cleanup_tasks = has_ordered_cleanup_tasks
def has_cleanup_task(self, *tasks: Performable) -> "Actor":
"""Alias for :meth:`~screenpy.actor.Actor.has_ordered_cleanup_tasks`."""
return self.has_ordered_cleanup_tasks(*tasks)

def with_cleanup_task(self, *tasks: Performable) -> "Actor":
"""Alias for :meth:`~screenpy.actor.Actor.has_ordered_cleanup_tasks`."""
return self.has_ordered_cleanup_tasks(*tasks)

def with_ordered_cleanup_tasks(self, *tasks: Performable) -> "Actor":
"""Alias for :meth:`~screenpy.actor.Actor.has_ordered_cleanup_tasks`."""
return self.has_ordered_cleanup_tasks(*tasks)
Comment on lines +96 to +98
Copy link
Member

@langgaibo langgaibo May 13, 2022

Choose a reason for hiding this comment

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

I propose we simply drop the singular variants of these methods because it's purely an English detail.
That would render a total of 6 aliases comprising both types of cleanup tasks.

(or even better, 4 total if we just drop the ordered aliases and rely on independent cleanup tasks to already be distinguishing themselves from the implied ordered ones... but that's a separate discussion.)

If we do want to explicitly alias singular and plural forms as you have here then we should also include all the options by adding with_cleanup_tasks, has_ordered_cleanup_task, with_ordered_cleanup_task, has_independent_cleanup_task, and with_independent_cleanup_task.
This would bring us to a total of 11 12 methods spanning both types of cleanup task!

Personally I'd rather just accept a format in which has_{type}_cleanup_tasks() may contain one or more tasks, but you know my position is that English readability doesn't have to mean native speaker english readability in terms of precise tense agreement

the_actor.has_ordered_cleanup_tasks(ClickButton())
the_actor.has_independent_cleanup_tasks(
    ClearTextField(),
    LogOut(),
)

Copy link
Member

Choose a reason for hiding this comment

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

TL:DR -

A) (my preference) Dropping the singulars would leave us with:

has_cleanup_tasks()
has_ordered_cleanup_tasks()
with_cleanup_tasks()
with_ordered_cleanup_tasks()

has_independent_cleanup_tasks()
with_independent_cleanup_tasks()

B) (something to consider) Also dropping the ordered variants and relying on the existing distinction already evident by the independent methods would leave us with

has_cleanup_tasks()
with_cleanup_tasks()

has_independent_cleanup_tasks()
with_independent_cleanup_tasks()

C) (english stickler) And going the other direction with all singular and plural options would leave us with

has_cleanup_task()
has_cleanup_tasks()
with_cleanup_task()
with_cleanup_tasks()
has_ordered_cleanup_task()
has_ordered_cleanup_tasks()
with_ordered_cleanup_task()
with_ordered_cleanup_tasks()

has_independent_cleanup_task()
has_independent_cleanup_tasks()
with_independent_cleanup_task()
with_independent_cleanup_tasks()

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we could go a different way with option (B) by dropping the less explicit form of the ordered variants and just stick with only the plurals of each e.g.

has_ordered_cleanup_tasks()
with_ordered_cleanup_tasks()

has_independent_cleanup_tasks()
with_independent_cleanup_tasks()

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, this is a great point. I think i agree with the streamline.


def has_independent_cleanup_tasks(self, *tasks: Performable) -> "Actor":
"""Assign one or more tasks for the Actor to perform when exiting.

The tasks included through this method are assumed to be independent;
that is to say, all of them will be executed regardless of whether
previous ones were successful.

Aliases:
* :meth:`~screenpy.actor.Actor.with_independent_cleanup_tasks`
"""
self.independent_cleanup_tasks.extend(tasks)
return self

with_independent_cleanup_tasks = has_independent_cleanup_tasks
def with_independent_cleanup_tasks(self, *tasks: Performable) -> "Actor":
"""Alias for :meth:`~screenpy.actor.Actor.has_independent_cleanup_tasks`."""
return self.has_independent_cleanup_tasks(*tasks)

def uses_ability_to(self, ability: Type[T]) -> T:
"""Find the Ability referenced and return it, if the Actor is capable.

Raises:
UnableToPerform: the Actor doesn't possess the Ability.

Aliases:
* :meth:`~screenpy.actor.Actor.ability_to`
"""
for a in self.abilities:
if isinstance(a, ability):
return a

raise UnableToPerform(f"{self} does not have the Ability to {ability}")

ability_to = uses_ability_to
def ability_to(self, ability: Type[T]) -> T:
"""Alias for :meth:`~screenpy.actor.Actor.uses_ability_to`."""
return self.uses_ability_to(ability)

def has_ability_to(self, ability: Type[Forgettable]) -> bool:
"""Ask whether the Actor has the Ability to do something."""
Expand All @@ -120,11 +152,22 @@ def has_ability_to(self, ability: Type[Forgettable]) -> bool:
return False

def attempts_to(self, *actions: Performable) -> None:
"""Perform a list of Actions, one after the other."""
"""Perform a list of Actions, one after the other.

Aliases:
* :meth:`~screenpy.actor.Actor.was_able_to`
* :meth:`~screenpy.actor.Actor.should`
"""
for action in actions:
self.perform(action)

was_able_to = should = attempts_to
def was_able_to(self, *actions: Performable) -> None:
"""Alias for :meth:`~screenpy.actor.Actor.attempts_to`, for test setup."""
return self.attempts_to(*actions)

def should(self, *actions: Performable) -> None:
"""Alias for :meth:`~screenpy.actor.Actor.attempts_to`, for test assertions."""
return self.attempts_to(*actions)

def perform(self, action: Performable) -> None:
"""Perform an Action."""
Expand Down Expand Up @@ -159,13 +202,29 @@ def cleans_up(self) -> None:
self.cleans_up_ordered_tasks()

def exit(self) -> None:
"""Direct the Actor to forget all their Abilities."""
"""Direct the Actor to forget all their Abilities.

Aliases:
* :meth:`~screenpy.actor.Actor.exit_stage_left`
* :meth:`~screenpy.actor.Actor.exit_stage_right`
* :meth:`~screenpy.actor.Actor.exit_through_vomitorium`
"""
self.cleans_up()
for ability in self.abilities:
ability.forget()
self.abilities = []

exit_stage_left = exit_stage_right = exit_through_vomitorium = exit
def exit_stage_left(self) -> None:
"""Alias for :meth:`~screenpy.actor.Actor.exit`."""
return self.exit()

def exit_stage_right(self) -> None:
"""Alias for :meth:`~screenpy.actor.Actor.exit`."""
return self.exit()

def exit_through_vomitorium(self) -> None:
"""Alias for :meth:`~screenpy.actor.Actor.exit`."""
return self.exit()
Comment on lines +207 to +217
Copy link
Member

Choose a reason for hiding this comment

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

Of course these aliases are strictly essential and must be preserved at all costs


def __repr__(self) -> str:
return self.name
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Callable, Generator, Any
from typing import Any, Callable, Generator
from unittest import mock

import pytest
Expand Down
19 changes: 15 additions & 4 deletions tests/test_actions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from unittest import mock
import os
import sys
from unittest import mock

import pytest

Expand All @@ -17,7 +18,6 @@
from screenpy.director import Director
from screenpy.exceptions import DeliveryError, UnableToAct, UnableToDirect
from screenpy.resolutions import IsEqualTo

from tests.conftest import mock_settings


Expand All @@ -27,6 +27,15 @@ def test_can_be_instantiated(self):

assert isinstance(atf, AttachTheFile)

def test_divines_filename(self):
filename = "thisisonlyatest.png"
filepath = os.sep.join(["this", "is", "a", "test", filename])
atf_without_path = AttachTheFile(filename)
atf_with_path = AttachTheFile(filepath)

assert atf_without_path.filename == filename
assert atf_with_path.filename == filename

@mock.patch("screenpy.actions.attach_the_file.the_narrator")
def test_perform_attach_the_file_sends_kwargs(self, mocked_narrator, Tester):
test_path = "souiiie.png"
Expand Down Expand Up @@ -121,8 +130,10 @@ def test_valueerror_when_poll_is_larger_than_timeout(self, Tester):
MockAction = self.get_mock_action()
ev = (
Eventually(MockAction)
.polling_every(200).milliseconds()
.for_(100).milliseconds()
.polling_every(200)
.milliseconds()
.for_(100)
.milliseconds()
)

with pytest.raises(ValueError) as actual_exception:
Expand Down
2 changes: 1 addition & 1 deletion tests/test_adapters.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import logging

from screenpy.narration.adapters.stdout_adapter import StdOutManager, StdOutAdapter
from screenpy.narration.adapters.stdout_adapter import StdOutAdapter, StdOutManager


def prop():
Expand Down
11 changes: 6 additions & 5 deletions tests/test_resolutions.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@
ContainsTheText,
ContainsTheValue,
DoesNot,
IsEmpty,
Equal,
HasLength,
IsCloseTo,
IsEmpty,
IsEqualTo,
IsNot,
ReadsExactly,
)


def assert_matcher_annotation(obj: BaseResolution):
assert type(obj.matcher) is obj.__annotations__['matcher']
assert type(obj.matcher) is obj.__annotations__["matcher"]


class TestBaseResolution:
Expand All @@ -40,6 +40,7 @@ def test_matcher_instantiation(self, args, kwargs, expected):

class MockResolution(BaseResolution):
"""Must be defined here for new mock matchers."""

matcher_function = mock.Mock()

resolution = MockResolution(*args, **kwargs)
Expand All @@ -54,12 +55,12 @@ class MockResolution(BaseResolution):
("describe_to", [mock.Mock()], "describe_to"),
("describe_match", [mock.Mock(), mock.Mock()], "describe_match"),
("describe_mismatch", [mock.Mock(), mock.Mock()], "describe_mismatch"),
]
],
)
def test_passthroughs(self, method, args, expected_method):

class MockResolution(BaseResolution):
"""Must be defined here for new mock matchers."""

matcher_function = mock.Mock()

resolution = MockResolution()
Expand All @@ -69,9 +70,9 @@ class MockResolution(BaseResolution):
getattr(resolution.matcher, expected_method).assert_called_once_with(*args)

def test___repr__(self):

class MockResolution(BaseResolution):
"""Must be defined here for new mock matchers."""

matcher_function = mock.Mock()
get_line = mock.Mock(return_value="")

Expand Down