Home Original page

Optional return type of `int` by GLMeece · Pull Request #717 · robotframework/SeleniumLibrary

@GLMeece

@GLMeece

aaltat

# Public, xpath

def get_matching_xpath_count(self, xpath):
def get_matching_xpath_count(self, xpath, return=str):

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

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

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

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

@aaltat

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

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

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

@aaltat

@GLMeece Would you have time to make the improvements?

@GLMeece

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

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