Adopt Bikeshed best practices and Infra spec conventions #495
Conversation
* Update callers to use dfn autolinks (for type checking) * Remove assertion about builder argument (simplify) * Remove test/exception for name argument (not needed) For webmachinelearning#450 and webmachinelearning#455
Note that many places still use [ ] notation; it may be better to convert those to non-normative text e.g. domintro sections, where the JS array-style notation is more common. For webmachinelearning#450.
WebIDL records are reflected in Infra as ordered maps, so can be iterated using "For each key → value of map", and the funky "element.key" syntax can be retired. For webmachinelearning#450
* Use "for each key → value of ..." syntax * Drop asserts for types For webmachinelearning#450 and webmachinelearning#455
Use links to the enum member definitions, rather than just strings. For webmachinelearning#450
Follow Infra conventions for updating internal slots. No need to use "reference" - it's implied. For webmachinelearning#450
Bikeshed makes definitions (algorithms, methods) and variables clickable and show valuable info. This collides with this spec's use of details elements to allow showing/hiding the algorithms, where clicking in the summary, by default, toggles the details. During setup of the spec, slip in a "preventDefault()" to make these play better together. Details in webmachinelearning#450
Infra style is "Let var be..." For webmachinelearning#450
* MLConvTranspose2dFilterOperandLayout * MLConv2dFilterOperandLayout For webmachinelearning#450
* MLDeviceType * MLGruWeightLayout * MLInterpolationMode * MLLstmWeightLayout * MLOperandDataType * MLPaddingMode * MLRecurrentNetworkDirection * MLRoundingType For webmachinelearning#450
Rather than `"opname"` just use "opname" consistently, which was more common. These are internal identifiers, not exposed to script. In the future it might be worth introducing a typographic convention and/or just a markup/markdown style for op names, just to make it easier to distinguish them. For webmachinelearning#450
E.g. if the key is a name, use |name| For webmachinelearning#450
With Bikeshed and autolinks you can just write "result of xxxing" For webmachinelearning#450
If Bikeshed [=autolinks=] are used instead of HTML <a>markup</a> then Bikeshed can do a better job of consistency checking. For webmachinelearning#450
| 1. Let |activation| be a new [=object=]. | ||
| 1. Set |activation|.{{MLActivation/[[builder]]}} to |builder|. | ||
| 1. Set |activation|.{{MLActivation/[[name]]}} to |name|. | ||
| 1. If |options| is an [=object=], set |activation|.{{MLActivation/[[options]]}} to |options|. | ||
| 1. If any of the following sub-steps fail, [=exception/throw=] an "{{OperationError}}" {{DOMException}}. | ||
| 1. Make a request to the underlying platform to: | ||
| 1. Create an [=implementation-defined=] platform operator |opImpl| for the given |name| operation. | ||
| 1. Store a reference of |opImpl| in |activation|.{{MLActivation/[[operator]]}}. | ||
| 1. Set |activation|.{{MLActivation/[[operator]]}} to |opImpl|. |
There was a problem hiding this comment.
@huningxin do we want to convey more here (and similar places) than just setting the internal slot?
There was a problem hiding this comment.
Just setting the internal slot might be fine. I suppose the graph build algorithm (to be specified) would fetch the platform operator from the internal slot at the graph building stage.
|
Thanks @inexorabletash, this was tedious work, but it looks much better now. |
|
FYI that I'm back from vacation now, and can address any comments from editors etc. Take your time! |
|
FYI that I've got another batch of similar fixups coming, but I'll do that as a second PR after this lands, for ease of reviewing. |
| 1. Let |activation| be a new [=object=]. | ||
| 1. Set |activation|.{{MLActivation/[[builder]]}} to |builder|. | ||
| 1. Set |activation|.{{MLActivation/[[name]]}} to |name|. | ||
| 1. If |options| is an [=object=], set |activation|.{{MLActivation/[[options]]}} to |options|. | ||
| 1. If any of the following sub-steps fail, [=exception/throw=] an "{{OperationError}}" {{DOMException}}. | ||
| 1. Make a request to the underlying platform to: | ||
| 1. Create an [=implementation-defined=] platform operator |opImpl| for the given |name| operation. | ||
| 1. Store a reference of |opImpl| in |activation|.{{MLActivation/[[operator]]}}. | ||
| 1. Set |activation|.{{MLActivation/[[operator]]}} to |opImpl|. |
There was a problem hiding this comment.
Just setting the internal slot might be fine. I suppose the graph build algorithm (to be specified) would fetch the platform operator from the internal slot at the graph building stage.
|
Is anything else needed here? Does this need discussion in the WG meeting? |
|
Thanks for this work. This sets an exemplary baseline to follow. No WG discussion needed for this PR if the editors have no further questions or comments. @huningxin or @wchao1115 please merge to unblock the next batch of similar improvements. |
|
I have no further questions. We usually need two approvals for a PR merge, I am not sure @wchao1115 has bandwidth on this PR. @anssiko or @zolkis , could you please review and approve? Thanks! |
There was a problem hiding this comment.
Well done @inexorabletash! We will note these improvements in the upcoming CR Snapshot. Other implementers watching appreciate these changes.
@zolkis can you resolve your comments to unblock your review? (Or let me know if GH does not let you do it.)
It was a simple comment, no need to resolve. I gave an explicit approval (even though that's an editors' privilege). |
|
@huningxin feel free to proceed to merge at your convenience. |
SHA: 0fe9e5c Reason: push, by huningxin Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
A bunch of relatively stand-alone commits addressing issues noted in #450
It'll be easiest to review each individual commit as they're each a separate "topic". Reviewers should feel free to cherry-pick changes if anything is deemed controversial.
💥 Error: 400 Bad Request 💥
PR Preview failed to build. (Last tried on Dec 19, 2023, 6:48 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.