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

Fix ReSpec definitions of RemotePlayback interface and other issues. #151

Merged
merged 9 commits into from
Sep 29, 2022

Conversation

markafoltz
Copy link
Contributor

@markafoltz markafoltz commented Sep 29, 2022

Converts references to terms to Bikeshed syntax.

It also:

  • Converts the use of void to undefined which was flagged by ReSpec
  • Adds a group tag to the ReSpec metadata
  • Stops linking user agent because we all know what that means

Preview | Diff

Copy link
Member

@tidoust tidoust left a comment

Choose a reason for hiding this comment

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

No problem with most of the simplifications to use more modern ReSpec shorthands in that pull request.

That said, I see the PR also updates the spec so that the IDL definitions end up in the IDL block. That is not the recommended way. At least, that's not the recommended approach in the ReSpec Documentation (FWIW, the Presentation API spec follows that recommendation).

You mention #137 but that change is not going to have any impact from a cross-reference perspective. Whether definitions are in an IDL block or in the prose, ReSpec understand they are IDL terms and will export them with the right attributes so that they can be picked up by tools that creates cross-references databases. Typically, IDL terms are already in ReSpec's cross-ref database and you can already use {{RemotePlayback}} from another ReSpec spec. These terms cannot be used from within a Bikeshed spec for now because the Remote Playback API is not part of Shepherd's crawl. Nothing prevents that and I requested addition some time ago in speced/bikeshed#1664

In other words, I would roll back on the updates that convert <dfn> </dfn> to {{ }} although you can indeed simplify them a bit (dropping useless backticks notably, e.g. <dfn data-dfn-for="RemotePlayback">watchAvailability()</dfn> should be enough).

Or are the changes intended because you prefer things that way?

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
markafoltz and others added 3 commits September 29, 2022 09:38
Co-authored-by: François Daoust <[email protected]>
Co-authored-by: François Daoust <[email protected]>
Co-authored-by: François Daoust <[email protected]>
@markafoltz
Copy link
Contributor Author

No problem with most of the simplifications to use more modern ReSpec shorthands in that pull request.

That said, I see the PR also updates the spec so that the IDL definitions end up in the IDL block. That is not the recommended way. At least, that's not the recommended approach in the ReSpec Documentation (FWIW, the Presentation API spec follows that recommendation).

I can try to follow that setup but I would like to understand how it works first; the ReSpec guide doesn't seem to explain how you can have multiple definitions of the same term.

You mention #137 but that change is not going to have any impact from a cross-reference perspective. Whether definitions are in an IDL block or in the prose, ReSpec understand they are IDL terms and will export them with the right attributes so that they can be picked up by tools that creates cross-references databases. Typically, IDL terms are already in ReSpec's cross-ref database and you can already use {{RemotePlayback}} from another ReSpec spec. These terms cannot be used from within a Bikeshed spec for now because the Remote Playback API is not part of Shepherd's crawl. Nothing prevents that and I requested addition some time ago in tabatkins/bikeshed#1664

From the issue it sounded like we shouldn't submit this spec for cross referencing yet because of issues with term definitions like RemotePlayback; @tabatkins says you can't define it with a plain <dfn>, which is what this PR is trying to resolve.

@tidoust
Copy link
Member

tidoust commented Sep 29, 2022

I can try to follow that setup but I would like to understand how it works first; the ReSpec guide doesn't seem to explain how you can have multiple definitions of the same term.

You cannot but there won't be multiple definitions of the same term. ReSpec is smart enough to generate <a> and not <dfn> in WebIDL blocks when the definitions already appear elsewhere in the spec.

From the issue it sounded like we shouldn't submit this spec for cross referencing yet because of issues with term definitions like RemotePlayback; @tabatkins says you can't define it with a plain <dfn>, which is what this PR is trying to resolve.

That's no longer relevant. Shepherd could not crawl the /TR version because it was too old at the time and did not yet follow the definitions data model. Also, since Shepherd does not run JavaScript, it cannot crawl raw (as opposed to generated versions of) ReSpec specs, so our Editor's Draft could not be added either. Since then, the /TR version was updated (and now matches the Editor's Draft) and we've been publishing the generated version of the spec to GitHub Pages in any case. Shepherd can crawl either version without problems.

FYI, there are a minority of specs that don't follow the "definitions in prose" pattern and rather have the definitions in the WebIDL block, or that use a mix of both approaches (for instance, the HTML spec has the interface definitions in the WebIDL blocks but methods definitions in the prose). That's why I suggested that this was up to you ;)

Note I'm happy to volunteer and update the PR to what I think would be the right result if you'd like me to!

@markafoltz
Copy link
Contributor Author

OK, converted to define WebIDL terms in prose. Is there anything we can do to expedite the term indexing in Shepherd?

Copy link
Member

@tidoust tidoust left a comment

Choose a reason for hiding this comment

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

Is there anything we can do to expedite the term indexing in Shepherd?

In theory, that should be a matter of adding a line somewhere in the right Shepherd table. I pinged Tab and Peter on the issue.

@markafoltz markafoltz merged commit 81d1d58 into main Sep 29, 2022
@markafoltz markafoltz deleted the fix-respec branch September 29, 2022 21:18
github-actions bot added a commit that referenced this pull request Sep 29, 2022
SHA: 81d1d58
Reason: push, by @mfoltzgoogle

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

2 participants