Skip to content

Optional return type of int#717

Closed
GLMeece wants to merge 1 commit intorobotframework:masterfrom
GLMeece:patch-3
Closed

Optional return type of int#717
GLMeece wants to merge 1 commit intorobotframework:masterfrom
GLMeece:patch-3

Conversation

@GLMeece
Copy link
Contributor

@GLMeece GLMeece commented Dec 17, 2016

Changed get_matching_xpath_count such that returning an integer is an option instead of only a string.

See also https://github.com/robotframework/Selenium2Library/issues/715

Changed `get_matching_xpath_count` such that returning an integer is an option instead of only a string.

See also https://github.com/robotframework/Selenium2Library/issues/715
# Public, xpath

def get_matching_xpath_count(self, xpath):
def get_matching_xpath_count(self, xpath, return=str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps return_str=True would be better argument definition.

  1. It would not be confused with Python return statement.
  2. Then you could do something like this (in lines 625 - 628):
    return str(count) if return_str else count

@pekkaklarck
Copy link
Member

First of all, it would be a good idea to test the code at least manually before submitting a PR. return=str isn't even valid syntax because return is a keyword in Python. I would assume this breaks importing the whole library. Additionally having default value str (built-in function in Python) and then comparing it against string 'str' is always going to yield false.

I also agree with @aaltat that specifying return type freely wouldn't give much benefits when there's only two meaningful values. Having something like return_string=True or return_integer=False would be better.

With Boolean values we should just think how to handle string 'False'. In Python that would be considered true (all non-empty strings are true), but in Robot data using return_string=False is much cleaner than using return_string=${FALSE}. How is this handled in general in S2L @aaltat? This is how it's handled in RF nowadays and I'd recommend same approach with S2L:
http://robotframework.org/robotframework/latest/libraries/BuiltIn.html#Boolean%20arguments

@aaltat
Copy link
Contributor

aaltat commented Dec 18, 2016

I didn't know that Robot Framework has such convince methods. The programmer me says that they are not needed but tester me like's it.

I don't remember do we have booleans somewhere in the keywords arguments, need to check it out.

@aaltat
Copy link
Contributor

aaltat commented Dec 18, 2016

There are three keywords, the Add Location Strategy and two alert keywords, which directly had True or False in their keyword signature. All those did use Python truth evaluation in their code and did not support Robot Framework more convenient style.

Perhaps supporting Robot Framework truth evaluation is a separate issue and for this PR using the Python truth evaluation is best way to go. I agree that Robot Framework truth evaluation is better and we should take it in use also in S2L.

@pekkaklarck
Copy link
Member

Yes, consistent handling of Boolean arguments is a separate issue. Here's the original RF issue: robotframework/robotframework#2038

@aaltat
Copy link
Contributor

aaltat commented Dec 19, 2016

Created issue #719 for handling booleans in same way as in core Robot Framework.

@GLMeece would you be willing to make the changes and write test(s) for the enhancement?

@GLMeece
Copy link
Contributor Author

GLMeece commented Dec 19, 2016

Mea culpa - I shouldn't have rushed this (I had family over, but that's no excuse). Yes, I'll work on this today, but it may be 10+ hours from now before I have a chance.

@aaltat
Copy link
Contributor

aaltat commented Dec 19, 2016

Take your time. It strongly looks like that we are not going to make a Xmas release.

@aaltat
Copy link
Contributor

aaltat commented Jan 11, 2017

@GLMeece Would you have time to make the improvements?

@GLMeece
Copy link
Contributor Author

GLMeece commented Jan 11, 2017

Hey, @aaltat - I have been slammed at work along with family issues (my parents are ill). At best, I might be able to take a look this weekend.

@aaltat
Copy link
Contributor

aaltat commented Jan 11, 2017

I understand, hope your parents get soon better. Pekka would like to start the new architecture planning/coding soon but first we would like to get some PR to the current master code line. And this PR is one of those PR. There is not rush and waiting to the weekend is totally fine. I was more interested on a status update.

And before you send the new commit, could you rebase with the current master branch. It contains CI improvements and now test are run automatically with Python 2&3 and selenium 2&3. But the Python 3 tests fails, because the Python 3 support is not yet completed.

@aaltat
Copy link
Contributor

aaltat commented Jan 22, 2017

Closed to favor of #735

@aaltat aaltat closed this Jan 22, 2017
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.

3 participants