-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[dotnet] Implement third-party Permissions module #16414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
[dotnet] Implement third-party Permissions module #16414
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No value,. In any case thank you.
dotnet/src/webdriver/BiDi/Extensions/Permissions/PermissionDescriptor.cs
Outdated
Show resolved
Hide resolved
| return bidi; | ||
| } | ||
|
|
||
| public static PermissionsModule AsPermissions(this BiDi bidi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely should be in another class. The reason is we are CLS compatible (I don't still understand why, but anyway), meaning some languages cannot deal with extension methods. They will use static method: WebDriverExtensions.AsPermissions(bidi). But it is not extension of WebDriver.
We should introduce good name for static class, and its namespace.
BiDiExtensions.AsPermissions(bidi)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Bluetooth module comes to under umbrella, then what? Should we have single static class for all them, or they define own static class in theirs namespace?
I vote for "everything in their namespace". It would be good pattern to demonstrate good practices how to develop module. Separate namespace is not a problem, IDE is still able to suggest extension method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extension methods do not affect CLS compliance. Users can call WebDriverExtensions.AsPermissions(bidi) even on extension methods.
But I agree that extensions should be in separate files. I will make those changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is about naming: WebDriverExtensions.AsPermissions(bidi) - it is not extension for WebDriver, it is for BiDi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the two suggestions: WebDriverExtensions -> BiDiExtensions, and permissions in its own area
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some libraries that wrap/extend Selenium that use the OpenQA.Selenium namespace segment, so it’s not a marginal concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My final confidence: # 1 - 65%, # 2 - 35%;
Please think and share your confidence (don't cheat).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me: # 1 - 75%, # 2 - 25%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YevgeniyShunevych please just vote blindly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view, extra namespaces within the same assembly, which do not have strong practical need, often provide only complexity without benefits. I'm for more clear type names, rather then giving short type names, which increases name conflict chance, and putting such types in different namespaces.
For example, Visual Studio 2022 IntelliSense, will not suggest you AsPermissions() extension method for bidi object unless you explicitly add using OpenQA.Selenium.BiDi.Permissions; to a .cs file before that. But I would like to see all the available "As..." methods in IDE without knowing and adding exact namespace before that.
I generally do support project folder structuring, similar as currently Selenium.WebDriver project is structured. It is also fine that we have all functionality under OpenQA.Selenium.BiDi namespace. But I would stop with sub-namespaces at this point. I don't see a strong need having 10+ sub-namespaces, one for each submodule. I am talking not only about extension classes, but all classes inside /dotnet/src/webdriver/BiDi folder.
Despite of namespace topic, the current BiDi folder is good. I'm ok with keeping methods like AsPermissions in separate extension classes inside corresponding folder ("Permissions"). If I had to choose a name for such extension class having method public static PermissionsModule AsPermissions(this BiDi bidi), I would name it PermissionsBiDiExtensions, not PermissionsExtensions, because the class's extension methods extend BiDi type. So just {Module}BiDiExtensions.
If you are not ready to apply single common OpenQA.Selenium.BiDi namespace for all types of /dotnet/src/webdriver/BiDi, it's understandable. But at least, it would be great to have {Module}BiDiExtensions in OpenQA.Selenium.BiDi namespace.
…eneral BiDi extensinos
User description
🔗 Related Issues
Fixes #15329
New API surface:
PR Type
Enhancement
Description
Enable external BiDi module creation and extension
Refactor module architecture with InternalModule base class
Expose Broker and JsonOptions for external access
Simplify module creation with public static method
Diagram Walkthrough
File Walkthrough
13 files
Expose internal properties and refactor module creationAdd new base class for internal modulesRefactor to support external module creationChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModuleChange inheritance to InternalModule