w3c / secure-payment-confirmation

Secure Payment Confirmation (SPC)
https://w3c.github.io/secure-payment-confirmation/
Other
106 stars 48 forks source link

Use lowercase values in enum #219

Closed dontcallmedom closed 1 year ago

dontcallmedom commented 1 year ago

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

dontcallmedom commented 1 year ago

specifically, this is about

 enum TransactionAutomationMode {
   "none",
-  "autoaccept",
-  "autoreject"
+  "autoAccept",
+  "autoReject",
+  "autoOptOut"
 };
rsolomakhin commented 1 year ago

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

dontcallmedom commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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

stephenmcgruer commented 1 year ago

Thanks @foolip - I've sent https://github.com/w3c/secure-payment-confirmation/pull/221 which hopefully fixes this in a way that works for everyone :)

javifernandez commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.