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

Stylistic change: Replace some explicit use of link target naming with scopes #499

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Jan 6, 2024

Simplify explicit link targets (e.g. power-preference-default) and link naming (e.g. default) with making the values scoped to a definition. Let Bikeshed take care of the linking, which also helps catch errors.

This PR layers on top of PR #495 since they touch similar lines, although they're conceptually separate. So only review the last commit here. Sorry about that!

This PR would also be partially obsoleted if we agree on #497 but it's harmless to merge as-is.


Preview | Diff

Copy link
Collaborator

@zolkis zolkis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not spot an error after two reads.
@huningxin PTAL, you might have better glasses. :)

As a general comment, we need to think if want to capture something more when changing constructs like

Store a reference to |outputImpl| in |output|.{{MLOperand/[[operand]]}}

to

Set |output|.{{MLOperand/[[operand]]}} to |outputImpl|.

@inexorabletash
Copy link
Member Author

In the context of https://github.com/webmachinelearning/webnn/blob/main/CONTRIBUTING.md just a note that the relevant commit here - fe38e88 - isn't even a stylistic change as it doesn't affect the visual appearance of the spec at all - it's purely Bikeshed twiddling.

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @inexorabletash !

@anssiko and @wchao1115 , please also take a look.

Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last and relevant commit fe38e88 LGTM, thanks!

@inexorabletash
Copy link
Member Author

Rebased so it's just got the one commit.

@inexorabletash inexorabletash changed the title Replace some explicit use of link target naming with scopes Stylistic change: Replace some explicit use of link target naming with scopes Jan 17, 2024
@wchao1115
Copy link
Collaborator

I don't mind this change but I'm curious about how to stay consistent throughout. It looks like this change replaces a few - with a whitespace. Is this what we want to go about identifier naming of the links going forward? How do we know if these are the only places in the doc that need this treatment?

@inexorabletash
Copy link
Member Author

So this comes down to a bit of a philosophical stance. I find that any time something explicit is used - like link target (lt) attributes, or a named identifier - rather than relying on the implicit name and Bikeshed's smarts, it requires extra typing and extra thought to maintain the spec.

There's nothing inherently wrong with the longer form - and in cases like providing an explicit scope for links I'm definitely a fan, but the cases tackled in this CL don't seem to add value.

Also... the changes proposed in #497 will actually overwrite several of the changes here with even more concise markdown, if that gets consensus.

Simplify explicit link targets (e.g. power-preference-default) and
link naming (e.g. default) with making the values scoped to a
definition. Let Bikeshed take care of the linking, which also helps
catch errors.
@inexorabletash
Copy link
Member Author

... but if we want to keep e.g. context-type as the internal ID I'm happy do update the PR. This would change <dfn>context type</dfn> to <dfn data-lt="context-type">context type</dfn> and then the scoped links become [=context-type/default=] and [=context-type/webgpu=].

The main intent of the PR was the scoping, so I'm happy either way.

@wchao1115
Copy link
Collaborator

@inexorabletash I have no problem with that reasoning but my question is more towards how to keep the exact naming convention you're envisioning consistent throughout the history of this spec and going forward given multiple contributors.

This is not different from defining a naming convention in a code project. Since naming consistency affects readability, it is most common for the project's contributors to agree on it once and for all, then codify it in writing e.g. NamingConvention.md, etc. Without such an agreement, next time someone makes a change, it'll break consistency again.

@inexorabletash
Copy link
Member Author

Ah, got it. The easy answer is: you don't invent IDs -- unless absolutely necessary, which is rare and usually means you're doing something wrong. If the spec text is "context type", that's what you link to or use as the scope. If the spec text is "default" ensure that it is defined within a scope and link to the scoped term.

I think some of the previous confusion came from having multiple "default" terms that weren't scoped so you needed to invent unique IDs for them, like "context-type-default". If the terms are properly scoped then you can just link to "context type/default" without having to define any arbitrary IDs.

@wchao1115
Copy link
Collaborator

Make sense. Would you take up my request to write a simple BikeShedIDNamingConvention.md (or feel free to pick a different file name) to describe how ID naming should be done in different situations with scoping sample? That way we can just have the existing Contributing.md pointing to it, as a convention for ID naming to avoid repeating the same mistake going forward. What do you think?

inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 21, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 21, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 22, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 22, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
anssiko pushed a commit that referenced this pull request Jan 24, 2024
…525)

As suggested by @wchao1115 in #499 and #518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
@inexorabletash
Copy link
Member Author

For completeness: text about scoping and link targets was included in the initial landing of SpecCodingConventions.md so this should be good to go.

@huningxin huningxin merged commit 7773794 into webmachinelearning:main Jan 25, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jan 25, 2024
SHA: 7773794
Reason: push, by huningxin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the scopeenums branch January 25, 2024 17:45
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 this pull request may close these issues.

5 participants