Target from parent#75
Conversation
|
I haven't added documentation yet. wanted your feedback before I did. |
|
I added some basic documentation that needs some refining. |
|
Well, I see a problem with the current design. Take the following example:: FIRST_NAME is mutated by calling |
bandophahita
force-pushed
the
target_from_parent
branch
from
d66147c to
f8774d7
Compare
perrygoy
left a comment
perrygoy
left a comment
There was a problem hiding this comment.
I had a bunch of thoughts, polish opportunities, and quibbles to talk through!
| | Websert clicks on the "Sign In" button. | ||
|
|
||
|
|
||
| Target in Target |
There was a problem hiding this comment.
polish: This won't affect the output, but i've been trying to write these .rst files following the Semantic Linebreak conventions, since it makes changes to these docs much easier to review in PRs. I won't say i've done a great job of it (i think i might have been a bit overzealous in the linebreaks in the past), but it's more than just a character limit.
Can you give these another pass and try to follow that convention?
There was a problem hiding this comment.
I am 100% terrible at figuring out where to break up the lines. I thought I was doing that. Better to just let me know where they should break.
| raise TargetingError(msg) from e | ||
|
|
||
| def inside(self, parent_target: Target) -> Self: | ||
| """Create a new Target and set the parent target for where search should start. |
There was a problem hiding this comment.
quibble: I think this might be getting too into the sauce for the first line. Maybe just say "Set the containing parent element for this Target" on the first line, then mention that we create and return a new Target.
We may also want to add an example of creating a parent-contained-Target in the docstring for Target above.
There was a problem hiding this comment.
I've updated the text... see if that works better.
| msg = f"{e} raised while trying to find {self}." | ||
| raise TargetingError(msg) from e | ||
|
|
||
| def inside(self, parent_target: Target) -> Self: |
There was a problem hiding this comment.
issue: This is not the correct typing for a method that returns a new object. Is there a generic "whatever class i am" return type, so we can support Target subclasses?
There was a problem hiding this comment.
I believe that technically it is correct even when it's not the same instance. I had the same thought as you. Self feels like an instance of, but iirc it's not limited to that.
There was a problem hiding this comment.
Using mypy and reveal_type shows that inside returns the subclass rather than the base class.
class SubTarget(Target):
pass
t1 = Target("t1").located((By.ID, "t1"))
st = SubTarget("s1").located((By.ID, "s1")).inside(t1)
reveal_type(st)Revealed type is "test_target.SubTarget" (232:16)
There was a problem hiding this comment.
Man, you're totally right! The docs confirm it: https://mypy.readthedocs.io/en/stable/more_types.html#precise-typing-of-alternative-constructors
I thought Self was very specifically "i return my self" but clearly it's a little more flexible than that. That's some good learnin', thanks for pushing back!
There was a problem hiding this comment.
(I was going to be very curious about why our mypy check wasn't yelling about it, i suppose that probably should have been my clue that i had some more learning to do.)
perrygoy
left a comment
perrygoy
left a comment
There was a problem hiding this comment.
I think you nailed the Semantic Linebreak approach, thanks for the quick rework! This looks good to me now. Nice add 👍
2 participants
Adding support in Target to find_elements within a found element.
Addresses #74