Conversation
|
typo fix commit is in this PR too |
hashtagchris
left a comment
There was a problem hiding this comment.
Looks good. Left some minor non-blocking comments.
| 'scriptpath', | ||
| ]) | ||
|
|
||
| static prepareSteps = Object.freeze([ |
There was a problem hiding this comment.
It would be good to have a comment that prepare's behavior is well-defined at this point and shouldn't grow or change beyond bug fixes.
| }), | ||
| }) | ||
| const { content } = await PackageJson.fix(path) | ||
| t.strictSame(content.name, '@npmcli/test-package') |
There was a problem hiding this comment.
Consider adding a fix then prepare test and confirming prepare is a noop with regard to fixed fields.
| t.strictSame(content.version, '1.0.0') | ||
| t.strictSame(content.bin, { 'test-package': '@npmcli/test-package' }) | ||
| t.strictSame(content.dependencies, { lodash: '' }) | ||
| t.strictSame(content.repository, { type: 'git', url: 'github.com/npm/test-package' }) |
There was a problem hiding this comment.
I'm probably opening a big can of worms, but should the repository url be prefixed with https:// or git+https://?
There was a problem hiding this comment.
That's a https://github.com/npm/hosted-git-info/ problem
There was a problem hiding this comment.
if (hosted) {
r = data.repository.url
= hosted.getDefaultRepresentation() === 'shortcut' ? hosted.https() : hosted.toString()
}We rely on that module to know if this was a "shortcut" or not, and change our representation accordingly.
No description provided.