Skip to content
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

Use lowercase values in enum #219

Closed
dontcallmedom opened this issue Dec 15, 2022 · 11 comments · Fixed by #221
Closed

Use lowercase values in enum #219

dontcallmedom opened this issue Dec 15, 2022 · 11 comments · Fixed by #221

Comments

@dontcallmedom
Copy link
Member

#215 introduced enum values that use camel-case; the convention for Web specs is to use lowercase, dash-separated values for enums

@dontcallmedom
Copy link
Member Author

specifically, this is about

 enum TransactionAutomationMode {
   "none",
-  "autoaccept",
-  "autoreject"
+  "autoAccept",
+  "autoReject",
+  "autoOptOut"
 };

@rsolomakhin
Copy link
Collaborator

Should it be "auto-accept", "auto-reject", "auto-opt-out"?

@dontcallmedom
Copy link
Member Author

These would be better, yes. Now, maybe you don't need the auto- prefix at all, if it's common to all values (except none)

@stephenmcgruer
Copy link
Collaborator

Interesting; we were advised to switch to camelcase by our internal WebDriver (well, ChromeDriver) team, to align with other such commands. I don't know if the specific person who recommended that interacts on GitHub much, but perhaps @foolip can suggest someone with WebDriver spec experience who can weight in on how enums for WebDriver should be cased?

@dontcallmedom
Copy link
Member Author

Oh, I had missed this was used in webdriver context. I'm not sure using webidl to describe a list of possible string values in webdriver makes sense in the first place...

@dontcallmedom
Copy link
Member Author

I'm not sure using webidl to describe a list of possible string values in webdriver makes sense in the first place

While noting that I'm not at all a WebDriver expert, looking at the WebDriver specification itself, these lists of possible values tend to be described as keyword tables, binding keywords with a state and a description. I think a likely reasonable match to what SPC is trying to do here is the known prompt handling approaches table .

I'll further note that while names for keys in the JSON objects used in WebDriver tend to use camelCase, the string values don't seem to be particularly using that convention.

@foolip
Copy link
Member

foolip commented Dec 16, 2022

There aren't many WebDriver experts in the world, and I wouldn't count myself amongst them, but I'll just go ahead and read https://w3c.github.io/webdriver/ and look for precedent.

Capabilities are camelCase: https://w3c.github.io/webdriver/#capabilities

Locator strategies are lowercase with spaces: https://w3c.github.io/webdriver/#locator-strategies

The Actions API uses camelCase: https://w3c.github.io/webdriver/#actions

https://w3c.github.io/permissions/#automation will use some lowercase-dashed identifiers from https://w3c.github.io/permissions-registry/, which are also used in JS APIs I think.

So my two suggestions would be:

  • Don't use Web IDL to define this, unless that enum is also used in some JS API and you do something similar to permissions.
  • Use camelCase if these strings will only appear in the WebDriver protocol.

But if @sadym-chromium or @nechaev-chromium say something different, then ignore me and listen to them :)

@stephenmcgruer
Copy link
Collaborator

Thanks @foolip - I've sent #221 which hopefully fixes this in a way that works for everyone :)

@javifernandez
Copy link

Hi, just chime in here to provide some context that may, or not, cause this issue to be reopened.

I'm implementing a new WebDridver extension command for the Custom Handlers specification. Since it's pretty similar to the Set SPC Transaction mode command, it was sensible to define a common enumeration for both as part of their ChromeDriver implementation [see the CL for details)

However, when submitting the PR to land the WPT actions and new tests, it was raised an issue about the inconsistency in the syntax of the the 'mode' parameter.

It's worth mentioning these Web Driver extensions commands are usually defined as part of the specification that needs them. In that sense, it's understandable that they may follow the syntax used in such spec. This implies a challenge in terms of consistency between the different specs. I guess that why the Web Platform Design Principles document is defined for, but I understand is not that easy as applying it blindly to any spec.

Getting back to the arguments give on this issue, considering now the casing rules defined in the mentioned design principles document, I see that the main argument given to keep camelCase was given in here, and summarized as follows:

Use camelCase if these strings will only appear in the WebDriver protocol.

It's true in both, RPH and SPC, commands the 'mode' argument use strings that are only used in the context of a WebDriver protocol (at least for now). So in that sense, we could follow that principle and change the Custom Handler spec. It's worth mentioning that HTML spec editors haven't risen strong opinions against, so it's just a matter of find a sensible consensus.

However, the cases described in the mentioned argument in favor of the camelCase are not quite the same than the enum value we are considering in both extension commands. The Capabilities and Actions are indeed dictionary keys; the casing rules mentioned before indeed suggest camel case of IDL dictionary keys, so in a way it follows the design principles document. On the other side, the document suggests 'lowercase, dash-delimited' syntax for enumeration values.

Are any of these considerations enough to reopen the issue and discuss it again ?
Thank you very much and sorry for getting attention on an already discussed topic.

@foolip
Copy link
Member

foolip commented Mar 15, 2023

@javifernandez you're right, https://w3c.github.io/webdriver/#capabilities isn't really a precedent here, because strings like "browserName" are keys in an object, not the values.

https://w3c.github.io/webdriver/#locator-strategies has lowercase space-delimted values.

I really don't have strong preferences, I was just trying to find some precedent. If all WebDriver implementers can agree on a principle, then clearly we should do whatever they say.

@javifernandez
Copy link

javifernandez commented Mar 15, 2023

Thanks for your opinion @foolip and thanks @annevk for filling the issue webdriver issue to try to get and agreement from WebDriver implementers. We can keep this issue closed and continue the discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants