-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(readme): Fixing Links #1228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fixes the the repoUrl
to be package url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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