Skip to content

Conversation

perrygoy
Copy link
Member

You know the drill—we're dropping Python 3.8 and adding Python 3.13. Sorry this one took me longer to get out—i had some typing things that i had to bellyache to myself about. I'll point them out in a comment below.

@perrygoy perrygoy requested a review from bandophahita July 22, 2025 04:39
Comment on lines 89 to 95
the_actor.attempts_to(AttachTheFile(self.path, **self.attach_kwargs))
the_actor.attempts_to(AttachTheFile(str(self.path), **self.attach_kwargs))
Copy link
Member Author

Choose a reason for hiding this comment

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

So, i want to update this to a Path in AttachTheFile, but i need to release screenpy first. Debating releasing ScreenPy, then updating this bit (and the bit in SaveScreenshot too), then releasing this. Gonna have to remember to do that so let's release 5.0 soon!

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want this to be Path, then I suggest removing str() here and let mypy complain until we release the screenpy update. In it's current state, the tests are failing because it's expecting Path in called args but getting a string.

opts.set_capability(
"deviceName", os.getenv("IOS_DEVICE_NAME", "iPhone Simulator")
"deviceName",
os.getenv("IOS_DEVICE_NAME", "iPhone Simulator"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we removed the ruff rule that was adding in the extra commas?

the_chain.move_to_element_with_offset(
self.target.found_by(the_actor), *self.offset
self.target.found_by(the_actor),
*self.offset,
Copy link
Contributor

Choose a reason for hiding this comment

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

ruff rule COM812 should not be in the configuration anymore. No need to add the comma here.

Copy link
Member Author

Choose a reason for hiding this comment

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

COMB812 is ignored; rerunning the lint does not change this back. Could there be a preference to list multiple arguments on separate lines when we need to linebreak for length? I haven't combed through the rules to find why this might be happening.

def describe(self) -> str:
"""Describe the Action in present tense."""
return f"Save browser console log as {self.filename}"
return f"Save browser console log as {self.path.name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused; after adding the property filename why change this line?

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 think i'm also confused. I thought i removed the filename property. 😵

I'll figure out what i meant here.

Copy link
Member Author

Choose a reason for hiding this comment

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

OH! I think this was a series of changes. I removed the filename set in __init__. Then realized the describe methods needed to describe the filepath, so i changed them to self.path.name. Then later decided to use a property for a different reason and made filename, but forgot to change the describe methods!

Thanks for your close attention; nice catch!


if self.attach_kwargs is not None:
the_actor.attempts_to(AttachTheFile(self.path, **self.attach_kwargs))
the_actor.attempts_to(AttachTheFile(str(self.path), **self.attach_kwargs))
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your comment above; we should not wrap self.path in str().

@perrygoy
Copy link
Member Author

As we expect, mypy is failing. This PR will need to wait for the ScreenPy release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants