Keyword "Set Browser Timeout": allow NONE or EMPTY to reset to the default

[simonmeggle]

In Set Selector Prefix, the argument ${NONE} or ${EMPTY} simply resets the prefix to the default (which is "empty").

=> I hereby want to request to adopt the behavior of Set Browser Timeout to behave the same way:

# not necessary anymore
${old}=  Set Browser Timeout  2
# (do something)
Set Browser Timeout  ${old}

# instead:
Set Browser Timeout  2
# (do something)
# I don't care what the old value was, just reset 
Set Browser Timeout  ${EMPTY} 

PS:
Perhaps also allow this one?
Set Browser Timeout default

[aaltat]

What would be default? The previous value timeout or the library default value (10seconds)?

[simonmeggle]

I would align the behavior with Set Selector Prefix - and empty value (or: "default") resets to the library default.
(An implicit reset to the previous value could be confusing because it can be hard to find out which value is valid before - in that case I think it's up to the user to store it in a variable.)

[aaltat]

This one should not be difficult to implement, would you like do a PR?

[falcon030]

@aaltat @simonmeggle Would it be an idea to just use
Set Browser Timeout
Without an argument? And that Set Browser Timeout uses the timeout argument with an default value of 10?

[aaltat]

That is also one possibility.

But we should apply the library import default value, instead we should the original imported value.

[falcon030]

@aaltat I've looked at the source code, but I don't see a variable for the default timeout. It looks like the default timeout is hardcoded in the init of the Browser class?
timeout: timedelta = timedelta(seconds=10),

[aaltat]

Yes it is. But I think we should not restore the library default value always. We should restore what value user did give (or the library default if nothing was given) in the library import. So we should save the import value (somewhere) and then restore saved value if Set Browser Timeout is used without arguments.

Example in below will restore the timeout to library default, 10s

*** Settings ****
Library    Browser

*** Tests ***
Example
    Set Browser Timeout

Second example in below will restore the timeout to 5s, the imported value.

*** Settings ****
Library    Browser     timeout=5s

*** Tests ***
Example
    Set Browser Timeout

In both cases what ever value the timeout argument has, is saved and used when Set Browser Timeout is used without arguments.

Using Set Browser Timeout ${None} or Set Browser Timeout ${EMPTY} is invalid and raises an exception.

[simonmeggle]

Sorry for being so quiet...
Set Browser Timeout alone looks a little bit awkward to me, it reads always that there is the most important information missing (the timeout itself).
Explicit is better than implicit - so, is there a reason against "default" as an argument?

[aaltat]

It depends how one expects keyword to work. It can have default value or it can return value from library import. The difference is that one can’t change the default value of the keyword, but one can change (if desired) library import value.

Both are good solutions to implement and although I am favoring explicit solutions, in this case resorting the library import offers better features for the users. By using library import users will always get the default value user did choose and not the value library maintainers did choose.

[falcon030]

@aaltat I think that what @simonmeggle is proposing is to use Set Browser Timout default to set the timeout back to the imported value or the default one when there is no imported value. But I might be wrong.
But in my opinion this will differ from the other keywords who have a optional timeout argument. These keywords resort to the last known timeout, when the timeout argument is not used.

[aaltat]

The last known good value is the library import value. Like I proposed in #3480 (comment) if keyword is used without an argument then keyword will use library import value. In this way it would work like other keywords.

[simonmeggle]

Yes, @aaltat the library import value (or if not given, the default) is what the reset should set.
My only point in the last comment was that it does not look intuitive to have just Set Browser Timeout.