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

Editorial: use step labels rather than numbers to refer to steps #2052

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jun 16, 2020

This fixes a persistent problem where step numbers would get stale and we'd forget to update them. Now, by given the steps (somewhat arbitrarily chosen) labels, we can refer and link to them explicitly. See tc39/ecmarkup#217 for a description of the syntax.

Marked as draft until a version of ecmarkup with support for the new syntax is published. Edit: done, this should be good to go. Deploy preview looks good to me.

Corresponding ecmarkdown PR at tc39/ecmarkdown#74. Corresponding ecmarkup PR at tc39/ecmarkup#217.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

etc. perhaps a better solution would be to make title automatic when the emu-xref is empty, and also hopefully allowing it to be self-closing (ie <emu-xref />)?

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@bakkot
Copy link
Contributor Author

bakkot commented Jun 16, 2020

The presence of the title attribute indicates that the title of the referent rather than its number should be used to provide the text of the reference. As such, title doesn't seem appropriate - steps do not have titles; they can only be referred to by number. Is there a different reason to add it?

and also hopefully allowing it to be self-closing

Ecma262 doesn't currently use any self-closing tags. I'm fine with discussing the possibility of adopting their use, but I would not want to do that in this PR.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

Sure, set that aside; maybe title is the wrong thing, but the contents of the emu-xref should be inferred somehow. Maybe a step attribute?

@bakkot
Copy link
Contributor Author

bakkot commented Jun 16, 2020

the contents of the emu-xref should be inferred somehow

They are; see the screenshot in tc39/ecmarkup#217, which corresponds to this file. That's the default behavior when you leave the tag empty.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

That's not what I see in this PR's build preview - maybe I just got confused because this doesn't include the ecmarkup PR yet?

@bakkot
Copy link
Contributor Author

bakkot commented Jun 16, 2020

Yeah, the build preview won't look right until support for the syntax it relies on is released.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

Would it be better to have an emu-step tag rather than an xref one then?

@brabalan
Copy link
Contributor

Thank you, this will be most useful for JSExplain. It might also solve our need to reference individual algorithms (#1800 and tc39/ecmarkup#172).

@bakkot
Copy link
Contributor Author

bakkot commented Jun 16, 2020

Would it be better to have an emu-step tag rather than an xref one then?

I don't see step references as fundamentally different from any other kind of thing you can emu-xref. What would be the advantage of having a separate tag?

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

Other kinds of xrefs can have custom link text which prevents automatically showing the reference's title; i don't see any use case for a link to a step without including a step number.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 16, 2020

That's true; I just made it a warning (to be promoted to a linting error) to include text. It doesn't seem like enough of a difference to warrant a different kind of element, to me.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

With the linting error i guess it's fine, but it seems conceptually like a different element to me. (we probably also want a warning on a non-step xref without title and without text?)

@bakkot
Copy link
Contributor Author

bakkot commented Jun 16, 2020

but it seems conceptually like a different element to me

How so?

we probably also want a warning on a non-step xref without title and without text?

That populates the link text with the clause number / figure number / etc, just as here it is populated with the step number; I don't see any particular problem with it. (We do that in ecma262.)

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

I think that "step" is a more specialized subtype of "a link", and i think that merits a different type.

It's not a blocking point, though - it was just an idea :-)

@bakkot bakkot marked this pull request as ready for review June 17, 2020 06:54
@bakkot
Copy link
Contributor Author

bakkot commented Jun 17, 2020

@ljharb Now that ecmarkup has been bumped the deploy preview renders correctly (see below). If that addresses your concern about title, would you resolve your comments above?

Screen Shot 2020-06-16 at 11 59 46 PM

@ljharb ljharb requested review from syg, michaelficarra and a team June 17, 2020 07:23
@ljharb ljharb self-assigned this Jun 17, 2020
@jmdyck
Copy link
Collaborator

jmdyck commented Jun 17, 2020

Currently, if you're looking at an <emu-xref href="#foo"> in the spec source, and want to go to the referenced point, you can do a search on foo (or, for less noise, id="foo"). This wouldn't work for the <emu-xref> elements introduced by this PR, because the full step-identifiers don't exist in the source. So consider changing the defining syntax from
[label="something"]
to
[id="step-something"]

Similarly, with regard to<emu-alg replaces-step="something">, consider that you might want to allow an emu document (e.g., a proposal) to replace a step in an algorithm in a different spec. For that, a reasonable syntax might be, for example:
<emu-alg replaces-step="https://tc39.es/ecma262/#step-something">
which suggests that the syntax for an in-spec reference should be:
<emu-alg replaces-step="#step-something">

In general, I think you should avoid using not-quite-the-real-identifier in source.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

I didn't confirm every single step ID was linked up right, since the tooling should have our backs here.

@ljharb ljharb added the editor call to be discussed in the next editor call label Jun 17, 2020
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

This looks great to me.

+1 on @jmdyck's searchability point. I'd prefer step labels not do any transformation of the label string.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 18, 2020

@jmdyck @syg Updated.

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

Successfully merging this pull request may close these issues.

None yet

6 participants