Remove assumption regarding local git clone #209
Conversation
| @@ -2,10 +2,6 @@ | |||
|
|
|||
| import { basename, dirname, join } from 'node:path'; | |||
| import { writeFile } from 'node:fs/promises'; | |||
| import { | |||
There was a problem hiding this comment.
Can you delete these utils if they're not used anywhere else?
|
|
||
| const tag = getCurrentGitHash(repositoryDirectory); | ||
| const tag = 'HEAD'; |
There was a problem hiding this comment.
Is it fine for us to assume HEAD?
There was a problem hiding this comment.
In dev it shouldn't really matter, but for releases it needs to be a hash since the line numbers referenced are going to change eventually.
I.e. for the latest v20 release, https://github.com/nodejs/node/blob/bb1a61d8737feff534bb85368dab3b7c554c863d/lib/_http_client.js#L869 is correct vs https://github.com/nodejs/node/blob/HEAD/lib/_http_client.js#L869 points to something else.
|
Fwiw the current behavior is almost directly taken from the old doc tooling https://github.com/nodejs/node/blob/63e3cc7e82a645b0763174f306bfe589164ce7d6/tools/doc/apilinks.mjs#L38-L45 |
There are numerous possibilities (env variables, CLI flag, some config file). Anyway, my point is that it’s better IMO to
Currently, this PR is not implementing the best solution, but I still think it’s an improvement |
|
I think the PR as it stands would break current behaviour. Let's not do that. I agree with opt-in, but then this PR needs to be updated to be able to do that. The changes should be relatively simple IMO (Like adding a new CLI argument and then conditionally invoking the git one based on that) |
IMO it's bad practice to assume the doc will be in a git directory, and to assume it would have an
originremote pointing to HTTPS or SSH GitHub repo. It would be much better to let the user opt-in into that behavior, and default to an hard-coded value.Description
Validation
Related Issues
Check List