Conversation
|
Looks good |
|
This is really good. Very clear. |
|
Should we update this to reflect: https://github.com/es-tooling/module-replacements/blob/main/.github/pull_request_template.md |
paoloricciuti
left a comment
paoloricciuti
left a comment
There was a problem hiding this comment.
Overall looks good...left a few comments but are mostly minor things
CONTRIBUTING.md
Outdated
| - The mapping generally does not need a `url` | ||
| - The replacement should have a `url` to its external documentation (e.g. MDN or Node docs) | ||
| - The replacement should have a `webFeatureId` if it is a web standard (these come from the [`web-features` dataset](https://github.com/web-platform-dx/web-features/tree/main/features)) | ||
| - The replacement should have a `nodeFeatureId` if it is a Node built-in (these come from the Node documentation) |
There was a problem hiding this comment.
Should we add an example here to make it easier to understand where to get the nodeFeatureId?
CONTRIBUTING.md
Outdated
| When using the `simple` type: | ||
|
|
||
| - The replacement should be of the form `snippet::{name}` where `name` is a suitable descriptive name for what the snippet does (e.g. `snippet:is-odd` for a snippet that checks if a number is odd) | ||
| - The replacement description should be a short human-readable description of what the snippet does, and how it replaces the old module |
There was a problem hiding this comment.
I remember having some issue with this when moving the old data so maybe we could have an example here?
CONTRIBUTING.md
Outdated
| - All manifests conform to the JSON schema | ||
| - Replacement keys match their `id` property | ||
| - `webFeatureId` values reference valid features and compat keys from the `web-features` dataset | ||
| - Mapping keys match their `moduleName` for module-type mappings | ||
| - Mappings only reference replacements that exist | ||
| - All documented modules (in `docs/modules/`) are listed in the modules README | ||
| - All documented modules have a corresponding entry in a manifest | ||
| - No module appears in multiple manifests | ||
| - Mappings and replacements are sorted alphabetically | ||
| - No replacement has `engines` set (these are populated automatically at publish time) | ||
| - All e18e URLs in mappings and replacements point to documentation files that exist |
There was a problem hiding this comment.
Do we need to keep this list here? It can easily go out of sync with the lint and if the errors from the scripts are readable it would be much easier to run the scripts and get the errors instead of reading this list.
There was a problem hiding this comment.
I think we can keep part of the list
|
@paoloricciuti i have updated it per your comments can you have another look? |
paoloricciuti
left a comment
paoloricciuti
left a comment
There was a problem hiding this comment.
I'm being veeeeery pedantic so feel free to fuck me off but I've left a couple more comments with suggestions 🧡
| - The mapping generally does not need a `url` | ||
| - The replacement should have a `url` to its external documentation (e.g. MDN or Node docs) | ||
| - The replacement should have a `webFeatureId` if it is a web standard (these come from the [`web-features` dataset](https://github.com/web-platform-dx/web-features/tree/main/features)) | ||
| - The replacement should have a `nodeFeatureId` if it is a Node built-in (these are of the form `{moduleName[, exportName]}`. For example, `fs` would be `{"moduleName": "fs"}`, while `fs.readFile` would be `{"moduleName": "fs", "exportName": "readFile"}`) |
There was a problem hiding this comment.
Why not this?
| - The replacement should have a `nodeFeatureId` if it is a Node built-in (these are of the form `{moduleName[, exportName]}`. For example, `fs` would be `{"moduleName": "fs"}`, while `fs.readFile` would be `{"moduleName": "fs", "exportName": "readFile"}`) | |
| - The replacement should have a `nodeFeatureId` if it is a Node built-in (these are of the form `{ moduleName: string, exportName?: string }`. For example, `fs` would be `{"moduleName": "fs"}`, while `fs.readFile` would be `{"moduleName": "fs", "exportName": "readFile"}`) |
|
|
||
| - The replacement should be of the form `snippet::{name}` where `name` is a suitable descriptive name for what the snippet does (e.g. `snippet:is-odd` for a snippet that checks if a number is odd) | ||
| - The replacement description should be a short human-readable description of what the snippet does, and how it replaces the old module | ||
| - The replacement description should be a short human-readable description of what the snippet does, and how it replaces the old module (e.g. "You can use arr.at(-1) to get the last element of an array") |
There was a problem hiding this comment.
Mmm I know I'm being pedantic but this doesn't look like the right example because it includes the snippets itself (and the descriptions for those should not include the snippet no?)
What about
| - The replacement description should be a short human-readable description of what the snippet does, and how it replaces the old module (e.g. "You can use arr.at(-1) to get the last element of an array") | |
| - The replacement description should be a short human-readable description of what the snippet does, and how it replaces the old module (e.g. "You can use a combination of filter and includes to calculate the difference between two arrays.") |
5 participants
🔗 Linked issue
📚 Description