add argument for get_list_items#721
add argument for get_list_items#721aaltat merged 10 commits intorobotframework:masterfrom qitaos:fix-select-list
Conversation
|
Thank you from the PR. Could you raise an issue about this, so that it makes the tracking the problem easier for maintainers. Also showing some examples which demonstrates the problem, would be really nice. Because I do not recall this type of problem. but I must say that I do not recall when was the last time I did use the keyword either. When we have are sure that we have a problem and what the problem actually is, we can think a solution for it. |
|
Looks OK from mobile screen but have to take closer look from bigger screen. |
aaltat
left a comment
There was a problem hiding this comment.
Code look OK also from big screen. But there is some documentation enhancements that should be done. Also some nit picking on test location.
| select lists are `id` and `name`. See `introduction` for details about | ||
| locating elements. | ||
|
|
||
| Sample: |
There was a problem hiding this comment.
We usually say Example: and not Sample:
|
|
||
| def get_list_items(self, locator): | ||
| """Returns the values in the select list identified by `locator`. | ||
| def get_list_items(self, locator, label=True): |
There was a problem hiding this comment.
It would be good to explain what the label argument actually does in the documentation.
test/acceptance/keywords/lists.robot
Outdated
| ... List 'interests' should have had no selection (selection was [ Males | Females | Others ]) | ||
| ... List Should Have No Selections interests | ||
|
|
||
| Get List Values From Single-Select List |
There was a problem hiding this comment.
Perhaps it would be good to place the test before the Get Selected List Value, then the test related to same keyword are in same location.
|
And you can add your name to the |
| """ | ||
| select, options = self._get_select_list_options(locator) | ||
| return self._get_labels_for_options(options) | ||
| if label: |
There was a problem hiding this comment.
This will be true if someone executes this keyword like label=False. A workaround is using label=${FALSE} (as shown in the docs), but that's ugly. RF itself handles these cases so that strings false and no, case-insensitively, are considered false. Was there @aaltat already an issue about handling Boolean arguments consistently within the library and, preferably, same way as Robot handles them? If there is, perhaps this could just be added there as one place to fix later.
Alternatively the signature could be changed to value=False. They you'd basically never wanted to pass explicit false value and using value=True on Robot would work.
There was a problem hiding this comment.
I like the later idea better at least in this PR
There was a problem hiding this comment.
I have changed to label=False, but it is true. maybe should fix the issue about handling Boolean arguments first.
There was a problem hiding this comment.
I think you did miss understood. Instead of using label=True argument (in line 11) we would like to use instead value=False argument. And then lines 25 - 26 would look like:
if value:
return self._get_values_for_options(options)
else:
return self._get_labels_for_options(options)
And this would solve the problem explained by @pekkaklarck. I would like to keywords work in same manner that users would not have think that in keyword X string False is actually true and in keyword Y string False is actually false. It could lead in confusing situations for the user point of view.
As for now, it is OK like this:
| Get List Items | xpath=//table | value=${FALSE} | # Returns labels |
| Get List Items | xpath=//table | value=FALSE | # Returns values |
place the test before the Get Selected List Value
aaltat
left a comment
There was a problem hiding this comment.
Did try to clarify in what way the PR should be changed.
Also my previous comments are not yet fixed.
| """ | ||
| select, options = self._get_select_list_options(locator) | ||
| return self._get_labels_for_options(options) | ||
| if label: |
There was a problem hiding this comment.
I think you did miss understood. Instead of using label=True argument (in line 11) we would like to use instead value=False argument. And then lines 25 - 26 would look like:
if value:
return self._get_values_for_options(options)
else:
return self._get_labels_for_options(options)
And this would solve the problem explained by @pekkaklarck. I would like to keywords work in same manner that users would not have think that in keyword X string False is actually true and in keyword Y string False is actually false. It could lead in confusing situations for the user point of view.
As for now, it is OK like this:
| Get List Items | xpath=//table | value=${FALSE} | # Returns labels |
| Get List Items | xpath=//table | value=FALSE | # Returns values |
|
I got it. @aaltat |
locator.", but it returns the labels. So fix the docstring to "Returns the labels in the select list identified bylocator."locator.".