robotframework / SeleniumLibrary

Web testing library for Robot Framework
Apache License 2.0
1.36k stars 751 forks source link

Discuss/Consider using all named arguments #1831

Open emanlove opened 1 year ago

emanlove commented 1 year ago

Coming from discussions [1] around combining all timeout library arguments and some changes done within the core and used by the browser library there is a thought/proposal to use all named arguments as compared to both named and positional ones within the library import.

[At the moment, this is more of a placeholder then a full description as I myself am still understanding the issue/proposal. But as a way to have an open discussion we want to place this here so we can start to describe, understand and discuss possibilities around this topic.]

[1] https://robotframework.slack.com/archives/C051AB7MFEH/p1681256514114099

Snooz82 commented 11 months ago

I am not able to read that Slack post. Maybe it was in private?

However:

Why did we (RF Browser) switch from pos_or_named arguments to named_only arguments on import and some keywords?

Browser library has some keyword that have a massive amount of arguments. Like New Browser or New Context or even more New Persistent Context

Also the library init has 13 arguments or so.

1. This is too much to use it positionally

calling these with positional arguments, the reader of this code would have no clue of which argument is what.

2. These arguments do not have any distinct order.

They are not related to each other. Like in SL: why is it so, that the action_chain_delay is the last argument? just because it was added last.

see also Click compared to new Click With Options

3. unordered positional arguments are harder to read and understand.

because there is no meaningful oder it is harder to find the correct argument.

4. positional arguments are hard to maintain or extend.

extending an argument interface of positional arguments always mean, to add new ones to the end and never get rid of any of those before. It is impossible to really deprecate an argument, because there is no way user may do the transition from one amount to another amount without breaking it. Exception is, that users use named arguments. then the order is irrelevant.

So named arguments are easy to extend and still keep a good shape/readability. Specially when the order of the args has anyway no meaning. See also Take Screenshot where the order of the named ones is irrelevant. However most of Browser libs getters do have a recognisable order of args.

How did we do the transition?

It was in my opinion a smart and effective solution.

  1. I created a *deprecated_pos_args argument, that would collect all positional arguments. here
  2. I ordered all arguments, which are now named_only, by their name and updated the docs.
  3. I created a dictionary of original order with names and types here
  4. I created some code that would iterate over these deprecated_pos_args assigned values, would interpret them by their position, convert them into the type the should have been and merge them with the named ones. see line 791 to 805. remark: line 804 params = dict(locals()) should have been the first thing in init before 789...

therefore you can maintain backwards compatibility and still log a deprecation warning. After a while you then can remove the *deprecated_pos_args