-
Notifications
You must be signed in to change notification settings - Fork 74
Improve linking for abstract&concrete methods #666
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
Conversation
|
This looks great, thank you! Can you also add a test which confirms that we now get signature-based errors for invocations of abstract methods? E.g. that if we have an abstract method |
src/ConcreteMethodDfns.ts
Outdated
| const li = spec.doc.createElement('li'); | ||
| const xrefNode = spec.doc.createElement('emu-xref'); | ||
| xrefNode.setAttribute('href', `#${def.refId}`); | ||
| xrefNode.textContent = def.for; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we prefix it with the section number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be possible to get that for free by re-using the existing emu-xref machinery by simply not populating textContent here. Alternatively, if that doesn't work because that phase has already run or something, we can export and re-use the relevant function directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not putting the text content seems to give just the number, without the text. I'll do it manually.
Would we want the number of the section (e.g. 16.2.1.2 Source Text Module Record) or of the specific concrete method that we are linking to (e.g. 16.2.1.7.2.2 Source Text Module Record)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going ahead with the full number, since that seems to be more useful when manually looking for the number (e.g. in a printed version), even if the title doesn't match.
Is this meant to warn because of the mismatched argument type? I do not seem to be getting that warning even with plain AOs. |
Sorry, has to be spelled "a String", with the capital letter, to actually work. Full example: <pre class="metadata">
copyright: false
</pre>
<emu-clause id="example" type="abstract operation">
<h1>
Example (
arg: a String,
): ~unused~
</h1>
<dl class="header"></dl>
</emu-clause>
<emu-alg>
1. Perform Example(~enum~).
</emu-alg> |
|
Oh yes that worked, thanks. |
michaelficarra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🆒
|
Looks great but should also get docs (under |
|
Updated docs but I want to make another couple tweaks first (making This PR will be a breaking change (because it enforces that |
|
OK, I'm happy with this, and got ecma262 building: tc39/ecma262#3745 |
Ref #391
The first commit:
<emu-table type="abstract methods">.ops to the biblio, so that autolinking will find their references and auto-link to the declarationThe second commit:
<emu-concrete-method-dfns for="SomeAbstractMethodName">, which auto-expands to a list of definitions forSomeAbstractMethodNameWith both together, this code:
Is rendered like this:
This PR does not introduce yet any checks that the signatures match.
Some questions:
<h1>, but I'm not sure it makes much sense to do it inside a table. Should I do it anyway for consistency?