Add more comments #1993
Add more comments #1993
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
|
|
Sure |
|
Hello @saschanaz |
|
I have updated the context to not call the API in the emitter, but that cause the test to fail, I will fix it later |
|
Done @saschanaz |
|
Hello @saschanaz, @jakebailey |
|
Hello @saschanaz |
|
I have updated the script to use promise @saschanaz |
|
Hello @saschanaz |
|
Hello @saschanaz |
|
Not sure I follow? Following the format of the current comments.json would be actually ideal, but I don't think this PR is currently doing that. The comment.json: {
"InterfaceName": {
"properties": {
"property": {
"foo": {
"comment": "(comment)"
}
}
}
}
}This PR right now: {
"InterfaceName.foo": "(comment)"
}Which looks more concise but with extra conversion step to the comment.json format anyway. Since we are automating it here it should directly emit the comment.json format. |
|
Sorry, i got confused. I will fix it |
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hello @saschanaz |
src/build/mdn-comments.ts
Outdated
| if (match) { | ||
| return match[1].replace(/ extension$/, "").split(":")[0]; | ||
| function generateSlug(content: string): string { | ||
| const match = content.match(/slug:\s*["']?([^"'\n]+)["']?/); |
There was a problem hiding this comment.
Is there anything that ^slug: (.+) can't cover?
There was a problem hiding this comment.
It can cover everything. I have updated it.
src/build/mdn-comments.ts
Outdated
| const parts = url.split("/").slice(2); // remove first 2 segments | ||
| const result = parts.join("/"); | ||
| return result; |
There was a problem hiding this comment.
It seems this is going to be split again, so maybe just return an array?
src/build/mdn-comments.ts
Outdated
| }); | ||
| } | ||
|
|
||
| function deepMerge( |
There was a problem hiding this comment.
Reuse merge() from build/utils.ts?
src/build/mdn-comments.ts
Outdated
| return result; | ||
| } | ||
|
|
||
| function urlToNestedObject(url: string, text: string): Record<string, any> { |
There was a problem hiding this comment.
Clever approach! I wonder we can skip merge() though, if we do something like:
function ensureLeaf(obj, keys) {
let leaf = obj;
for (const key of keys) {
leaf[key] ??= {};
leaf = leaf[key]
}
return leaf;
}Then we can do:
const result = {};
const leaf = ensureLeaf(result, slug); // e.g. when slug is ["AbortSignal", "abort"]
leaf.__comment = "(comment)"|
|
||
| // Add comments to properties | ||
| for (const prop of propertyItems) { | ||
| const propDesc = descObject[prop.name]; |
There was a problem hiding this comment.
This is still less than ideal, the ideal would be that being able to just merge() here.
We can extract whether it's property or method from the page-type: metadata, e.g. web-api-instance-property, web-api-instance-method, web-api-static-property, web-api-static-method, etc.
With that we could make the generateSlug below to generate the keys for full Browser.Interface-compliant structure, e.g. ["AbortSignal", "properties", "property", "aborted"].
If the full compliance is achieved then we can just do merge() here.
(Unfortunately MDN doesn't have much to tell whether it's for namespace though. We should keep the current line 141.)
There was a problem hiding this comment.
Hello @saschanaz
I have tried to use merge, but when I use merge it returns a bunch of errors for the emitter, like:
Error: Missing 'type' field in {"signal":"The **`signal`** read-only property of the AbortController interface returns an AbortSignal object instance, which can be used to communicate with/abort an asynchronous operation as desired."}
I think we can't use it
There was a problem hiding this comment.
The object looks like it's not using { "comment": "..." }, in that case it would overwrite on existing object and fail
related to #1940
Hello:)
In my last PR I have used MDN to generate comments for interface declarations, in this pr I add it for the properties.