Accept all text-like input fields in Textfield-related keywords #817
Conversation
| @@ -192,6 +194,8 @@ def _element_matches(self, element, tag, constraints): | |||
| if not element.tag_name.lower() == tag: | |||
| return False | |||
| for name in constraints: | |||
| if type(constraints[name]) is list and element.get_attribute(name) not in constraints[name]: | |||
| return False | |||
There was a problem hiding this comment.
- Why is this change needed?
- Should use
usinstance, nottyoe
There was a problem hiding this comment.
Why is this change needed?
Currently constraints only accept 1 value per element attribute and the change involves the creation of a list of values that should be considered acceptable.
Should use usinstance, not tyoe
Will do the change. Never coded in Python so I really don't know what's best. :) Thanks for the review! 👍
|
I'm +1 for this enhancement but tests are needed and this should be mentioned in docs too. |
|
And raise issue also from the problem, so that release notes are properly created. |
|
Ah, I did not remember that one, thanks Pekka. In any case, the changes needs to be tested. In in unit and/or acceptance level. Also the change breaks the existing unit test which is not acceptable. For the code side, I do not have other comments than Pekka did made. Could you fix the testing side related problems and lets see how it continues from there. |
For sure! When I opened the pull request I was busy at work. I was hoping somebody experienced would take it over. In any case, I have some time know and I'm gonna take the time to learn a bit about testing in Python. If I cannot successfully finish it, I'll let you guys know. |
|
Could you please have a second look? I hope that's enough, if it's not, please let me know how I can improve. :) Thanks! |
|
I'm on vacation without my laptop next week (and currently trying to get everything packed). I can take a quick look at the code now, but hopefully @aaltat has time for a full review. |
| else: | ||
| contraint = "@%s='%s'" % (name, value) | ||
| xpath_constraints.append(contraint) | ||
| return xpath_constraints |
There was a problem hiding this comment.
This seems to be totally fine, but if there would be _get_xpath_constraint, looping could be handled by a list comprehension similarly as earlier:
xpath_constraints = [self._get_xpath_constraint(name, value)
for name, value in constraints.items()]There was a problem hiding this comment.
Made this change too. It's much better. I was just afraid of creating a new method because all other methods in the file are relatively long. I really like having smaller and more methods. ^^
| if isinstance(constraints[name], list): | ||
| if element.get_attribute(name) not in constraints[name]: | ||
| return False | ||
| elif not element.get_attribute(name) == constraints[name]: |
There was a problem hiding this comment.
Using != would be more clear than not and ==.
There was a problem hiding this comment.
I agree but I thought you Python guys did that (based on the line removed above). Change made! :)
|
I can take a look at this week. |
|
And I was sick last week, I will try to find time this week. |
|
By looking from mobile screen, this looks OK. I need to give second look from a bigger screen. In mean while, could you rebase with master and create a PR which does not contain conflicts? |
|
All looks OK to me. Would you mind sorting out the conflict and it can be merged to the master? |
|
@aaltat On it! Sorry it took me so long to reply. Weekend's here now! 💃 |
|
@aaltat Rebased and Travis is green! ✅ 😄 |
|
No problem. I will try to take a look later and if all is well then merge to master. |
This fixes #546