fix(readme): Fixing Links #1228
Conversation
There was a problem hiding this comment.
self-review, @achingbrain looks like, there was a breaking change npx aegir check-project (the current latest) seems to break READMEs, looking into that.
| @@ -399,7 +399,7 @@ async function processModule (projectDir, manifest, branchName, repoUrl, homePag | |||
| } | |||
|
|
|||
| await checkLicenseFiles(projectDir) | |||
| await checkReadme(projectDir, repoUrl, branchName, rootManifest) | |||
| await checkReadme(projectDir, homePage, branchName, rootManifest) | |||
There was a problem hiding this comment.
this fixes the the repoUrl to be package url.
There was a problem hiding this comment.
If this is the fix that is required, please rename this variable throughout checkReadme - repoUrl is https://github.com/ipfs/helia but homePage is https://github.com/ipfs/helia/tree/master/packages/helia.
To expect one and receive the other is a recipe for complete chaos.
Also, the homePage generation code has master hard-coded as the branch name - it should read it from the repo config instead since we don't use that everywhere any more.
| (options: ReadmeStringGeneratorInputOptions): string | ||
| } | ||
|
|
||
| interface ReadmeStringGenerator { |
There was a problem hiding this comment.
npm already turns relative links into absolute links on module pages so it's not clear to me if this solves an extant problem? Did you run into something?
More generally - please can you use the fix: tag for changes like this in future, since this is a bug fix. feat: is for new features and will appear in the Features section of the release notes.
Please also use the active voice for PR titles - e.g. fix: link to license files on github - it's usually more concise than the passive voice and makes for better bullet points in the release notes. If in doubt, just say what the PR does 😉
| const license = parseMarkdown(LICENSE(pkg, repoOwner, repoName, defaultBranch)) | ||
| const license = parseMarkdown(LICENSE({ repoUrl, repoOwner, repoName })) | ||
| const apiDocs = parseMarkdown(APIDOCS(pkg)) | ||
| const structure = parseMarkdown(STRUCTURE(projectDir, projectDirs)) |
There was a problem hiding this comment.
I agree that passing an object for options can make the code more flexible, but now the argument types for HEADER, LICENSE, APIDOCS and STRUCTURE are inconsistent, since some take an object and some take multiple strings.
Please can you make them consistent - either leave it as it was with multiple string arguments, or convert them all to take option arguments.
| return ` | ||
| # ${pkg.name} <!-- omit in toc --> | ||
| # [${pkg.name}](${repoUrl}) <!-- omit in toc --> |
There was a problem hiding this comment.
Every module on npm has a Repository link that is generated from the package.json repository field and it's not very common to turn the module name into a link.
Turning this into a link also messes up the TOC generation as it takes the <!-- omit in toc --> to be part of the title rather than a directive to omit it.
I think this is supposed to solve:
NPM docs like don't link back to the top-level helia REAME which has all the info (see npmjs.com/package/helia ). The API links from within are broken.
from ipfs/helia#35 but I think we'd be better off adding a Documentation section to each package in the helia monorepo that links to the relevant sources.
There was a problem hiding this comment.
for a package the repository points the repository as it should, not the package link. e.g. https://www.npmjs.com/package/@helia/interface @helia/interface should point to the license and path of the package.
I can instead modify the homepage url instead.
| @@ -399,7 +399,7 @@ async function processModule (projectDir, manifest, branchName, repoUrl, homePag | |||
| } | |||
|
|
|||
| await checkLicenseFiles(projectDir) | |||
| await checkReadme(projectDir, repoUrl, branchName, rootManifest) | |||
| await checkReadme(projectDir, homePage, branchName, rootManifest) | |||
There was a problem hiding this comment.
If this is the fix that is required, please rename this variable throughout checkReadme - repoUrl is https://github.com/ipfs/helia but homePage is https://github.com/ipfs/helia/tree/master/packages/helia.
To expect one and receive the other is a recipe for complete chaos.
Also, the homePage generation code has master hard-coded as the branch name - it should read it from the repo config instead since we don't use that everywhere any more.
|
Closing as this is very old any possibly not required any more. Please re-open and update if you disagree. |
To implement: ipfs/helia#77