SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
30.8k stars 8.2k forks source link

[🚀 Feature]: implement namespacing methods as properties #14065

Open titusfortner opened 5 months ago

titusfortner commented 5 months ago

Feature and motivation

.NET copied the Java approach of using methods for namespacing:

driver.Manage().Cookies().AddCookie(cookie)

When a more idiomatic way would be to use properties. This came up when discussing adding the BiDi functionality. Rather than mixing them, or using the old way, I propose we add the properties and deprecate the methods.

Usage example

driver.Manage.Cookies.AddCookie(cookie)
YevgeniyShunevych commented 5 months ago

This is a good improvement. I propose also to remove, redundant from my point of view, Manage.

driver.Cookies.AddCookie(cookie)
nvborisenko commented 5 months ago

Let's think.

driver.Cookies.AddCookie(cookie)

Cookies looks like a state. Hmm, yes, but remote state. So driver.Cookies property looks good. Let's improve it.

driver.Cookies.Add(cookie); // instead of AddCookie

It is possible, just make Cookies as IList<Cookie>, we can mimic adding and removing cookies. Looks promising. But what about Cookie class, is it immutable or mutable. If mutable - then any modification should be reflected on server side.

We have many methods like Manage(), Cookies(), 'Navigate()'... I already proposed something like driver.AsNavigable(). The same approach could be applied for driver.AsCookable() :D (please find better naming)

titusfortner commented 5 months ago

We need to be careful about making large scale changes to the API. It's one thing to move from a method to a property, but rewriting everything is a lot to ask for users if we want them to keep their code up to date.

nvborisenko commented 5 months ago

From my point of view it is better to break once and announce this breaking change, rather than break, break, break...

YevgeniyShunevych commented 5 months ago

It is possible, just make Cookies as IList, we can mimic adding and removing cookies.

Unfortunately, "cookie jar" can not implement IList<Cookie> as Add and other methods should become async anyway. So it can probably be either:

await driver.Cookies.AddCookieAsync(cookie);

or:

await driver.Cookies.AddAsync(cookie);

I vote for the second one.


But what about Cookie class, is it immutable or mutable.

Currently it is immutable https://github.com/SeleniumHQ/selenium/blob/trunk/dotnet/src/webdriver/Cookie.cs.


From my point of view it is better to break once and announce this breaking change, rather than break, break, break...

Agree. This the best moment for such a refactoring if the API is breaking anyway.

RenderMichael commented 1 month ago

This would be a nice improvement! The current approach caught me off guard.

Normally properties should be nouns, so what about from:

driver.Manage().Cookies().AddCookie(cookie);

to

driver.Manager.Cookies.Add(cookie);

Same thing Navigate() -> Navigator.

It would be my preference to change driver.SwitchTo().Frame(frame) or driver.SwitchTo().Frame(frame) to single methods:

driver.SwitchToFrame(frame);
driver.OpenNewWindow(...);

But I recognize this is much bigger change (still, this would be the time)