add argument for get_list_items #721

Merged
merged 10 commits into from Jan 11, 2017

Projects

None yet

3 participants

@qitaos
Contributor
qitaos commented Dec 27, 2016
  1. the keyword get_list_items docstring wrote "Returns the values in the select list identified by locator.", but it returns the labels. So fix the docstring to "Returns the labels in the select list identified by locator."
  2. add the keyword get_list_values. It will "Returns the values in the select list identified by locator.".
@aaltat
Contributor
aaltat commented Dec 27, 2016

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.

@qitaos
Contributor
qitaos commented Dec 28, 2016

@aaltat I rase an issue #722 about this. Thank you.

@qitaos qitaos changed the title from add keyword get_list_values to add argument for get_list_items Dec 30, 2016
@aaltat
Contributor
aaltat commented Dec 31, 2016

Looks OK from mobile screen but have to take closer look from bigger screen.

@aaltat

Code look OK also from big screen. But there is some documentation enhancements that should be done. Also some nit picking on test location.

@@ -8,15 +8,24 @@ class _SelectElementKeywords(KeywordGroup):
# Public
- def get_list_items(self, locator):
- """Returns the values in the select list identified by `locator`.
+ def get_list_items(self, locator, label=True):
@aaltat
aaltat Jan 2, 2017 Contributor

It would be good to explain what the label argument actually does in the documentation.

Select list keywords work on both lists and combo boxes. Key attributes for
select lists are `id` and `name`. See `introduction` for details about
locating elements.
+
+ Sample:
@aaltat
aaltat Jan 2, 2017 Contributor

We usually say Example: and not Sample:

@qitaos
qitaos Jan 3, 2017 Contributor

Fixed it

test/acceptance/keywords/lists.robot
@@ -148,6 +148,18 @@ List Should Have No Selections
... 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
@aaltat
aaltat Jan 2, 2017 Contributor

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.

@qitaos
qitaos Jan 3, 2017 Contributor

Fixed it

@aaltat
Contributor
aaltat commented Jan 2, 2017

And you can add your name to the BUILD.rst file if you want.

"""
select, options = self._get_select_list_options(locator)
- return self._get_labels_for_options(options)
+ if label:
@pekkaklarck
pekkaklarck Jan 2, 2017 Member

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.

@aaltat
aaltat Jan 2, 2017 Contributor

I like the later idea better at least in this PR

@qitaos
qitaos Jan 3, 2017 edited Contributor

I have changed to label=False, but it is true. maybe should fix the issue about handling Boolean arguments first.

@aaltat
aaltat Jan 3, 2017 Contributor

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 |
@aaltat

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:
@aaltat
aaltat Jan 3, 2017 Contributor

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 |
qitaos added some commits Jan 11, 2017
@qitaos qitaos modify argument label=True to value=False
modify argument label=True to value=False
and the test cases.
74049a4
@qitaos qitaos Revert "modify argument label=True to value=False"
This reverts commit 74049a4.
a0ca5e9
@qitaos qitaos Merge branch 'fix-select-list' of https://github.com/qitaos/Selenium2…
…Library into fix-select-list
9ba0272
@qitaos qitaos modify argument label=True to value=False
modify argument label=True to value=False
and modify the cases.
215030a
@qitaos
Contributor
qitaos commented Jan 11, 2017

I got it. @aaltat

@aaltat aaltat merged commit 7a8d14a into robotframework:master Jan 11, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment