Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(readme): Fixing Links #1228

Closed
wants to merge 4 commits into from
Closed

fix(readme): Fixing Links #1228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edb9f80
feat: :sparkles: LICENSE links
whizzzkid Apr 6, 2023
587b049
feat: :sparkles: Fixing HEADER Generator
whizzzkid Apr 6, 2023
ab0efc4
fix: :recycle: Simplifying Types
whizzzkid Apr 6, 2023
782adbf
fix: :wastebasket: Fixing unneeded methods
whizzzkid Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/check-project/check-monorepo-readme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export async function checkMonorepoReadme (projectDir, repoUrl, defaultBranch, p
const file = parseMarkdown(readmeContents)

// create basic readme with heading, CI link, etc
const readme = parseMarkdown(HEADER(pkg, repoOwner, repoName, defaultBranch))
const readme = parseMarkdown(HEADER({ defaultBranch, pkg, repoOwner, repoName, repoUrl }))

// remove existing header, CI link, etc
/** @type {import('mdast').Root} */
Expand Down Expand Up @@ -121,7 +121,7 @@ export async function checkMonorepoReadme (projectDir, repoUrl, defaultBranch, p
parsedReadme.children.push(child)
})

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))
Comment on lines -124 to 126
Copy link
Member

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.


Expand Down
4 changes: 2 additions & 2 deletions src/check-project/check-readme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export async function checkReadme (projectDir, repoUrl, defaultBranch, rootManif
const file = parseMarkdown(readmeContents)

// create basic readme with heading, CI link, etc
const readme = parseMarkdown(HEADER(pkg, repoOwner, repoName, defaultBranch))
const readme = parseMarkdown(HEADER({ defaultBranch, pkg, repoOwner, repoName, repoUrl }))

// remove existing header, CI link, etc
/** @type {import('mdast').Root} */
Expand Down Expand Up @@ -141,7 +141,7 @@ export async function checkReadme (projectDir, repoUrl, defaultBranch, rootManif

const installation = parseMarkdown(INSTALL(pkg))
const apiDocs = parseMarkdown(APIDOCS(pkg, rootManifest))
const license = parseMarkdown(LICENSE(pkg, repoOwner, repoName, defaultBranch))
const license = parseMarkdown(LICENSE({ repoOwner, repoName, repoUrl }))

parsedReadme.children = [
...installation.children,
Expand Down
2 changes: 1 addition & 1 deletion src/check-project/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

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.

Copy link
Member

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.

}

export default new Listr([
Expand Down
22 changes: 10 additions & 12 deletions src/check-project/readme/header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,39 @@
/**
* @type {Record<string, (repoOwner: string, repoName: string, defaultBranch: string) => string>}
* @typedef {import('../../types').ReadmeStringGeneratorInputOptions} ReadmeStringGeneratorInputOptions
* @type {Record<string, (options: Omit<ReadmeStringGeneratorInputOptions, 'repoUrl'> & { defaultBranch: string}) => string>}
*/
const BADGES = {
libp2p: (repoOwner, repoName, defaultBranch) => `
libp2p: ({ repoOwner, repoName, defaultBranch }) => `
[![libp2p.io](https://img.shields.io/badge/project-libp2p-yellow.svg?style=flat-square)](http://libp2p.io/)
[![Discuss](https://img.shields.io/discourse/https/discuss.libp2p.io/posts.svg?style=flat-square)](https://discuss.libp2p.io)
[![codecov](https://img.shields.io/codecov/c/github/${repoOwner}/${repoName}.svg?style=flat-square)](https://codecov.io/gh/${repoOwner}/${repoName})
[![CI](https://img.shields.io/github/actions/workflow/status/${repoOwner}/${repoName}/js-test-and-release.yml?branch=${defaultBranch}&style=flat-square)](https://github.com/${repoOwner}/${repoName}/actions/workflows/js-test-and-release.yml?query=branch%3A${defaultBranch})
`,
ipfs: (repoOwner, repoName, defaultBranch) => `
ipfs: ({ repoOwner, repoName, defaultBranch }) => `
[![ipfs.tech](https://img.shields.io/badge/project-IPFS-blue.svg?style=flat-square)](https://ipfs.tech)
[![Discuss](https://img.shields.io/discourse/https/discuss.ipfs.tech/posts.svg?style=flat-square)](https://discuss.ipfs.tech)
[![codecov](https://img.shields.io/codecov/c/github/${repoOwner}/${repoName}.svg?style=flat-square)](https://codecov.io/gh/${repoOwner}/${repoName})
[![CI](https://img.shields.io/github/actions/workflow/status/${repoOwner}/${repoName}/js-test-and-release.yml?branch=${defaultBranch}&style=flat-square)](https://github.com/${repoOwner}/${repoName}/actions/workflows/js-test-and-release.yml?query=branch%3A${defaultBranch})
`,
multiformats: (repoOwner, repoName, defaultBranch) => `
multiformats: ({ repoOwner, repoName, defaultBranch }) => `
[![multiformats.io](https://img.shields.io/badge/project-IPFS-blue.svg?style=flat-square)](http://multiformats.io)
[![codecov](https://img.shields.io/codecov/c/github/${repoOwner}/${repoName}.svg?style=flat-square)](https://codecov.io/gh/${repoOwner}/${repoName})
[![CI](https://img.shields.io/github/actions/workflow/status/${repoOwner}/${repoName}/js-test-and-release.yml?branch=${defaultBranch}&style=flat-square)](https://github.com/${repoOwner}/${repoName}/actions/workflows/js-test-and-release.yml?query=branch%3A${defaultBranch})
`,
default: (repoOwner, repoName, defaultBranch) => `
default: ({ repoOwner, repoName, defaultBranch }) => `
[![codecov](https://img.shields.io/codecov/c/github/${repoOwner}/${repoName}.svg?style=flat-square)](https://codecov.io/gh/${repoOwner}/${repoName})
[![CI](https://img.shields.io/github/actions/workflow/status/${repoOwner}/${repoName}/js-test-and-release.yml?branch=${defaultBranch}&style=flat-square)](https://github.com/${repoOwner}/${repoName}/actions/workflows/js-test-and-release.yml?query=branch%3A${defaultBranch})
`
}

/**
* @param {*} pkg
* @param {string} repoOwner
* @param {string} repoName
* @param {string} defaultBranch
* @type {import('../../types').ReadmeStringGenerator}
*/
export const HEADER = (pkg, repoOwner, repoName, defaultBranch) => {
export const HEADER = ({ defaultBranch, pkg, repoOwner, repoName, repoUrl }) => {
return `
# ${pkg.name} <!-- omit in toc -->
# [${pkg.name}](${repoUrl}) <!-- omit in toc -->
Copy link
Member

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.

Copy link
Contributor Author

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.


${(BADGES[repoOwner] ?? BADGES.default)(repoOwner, repoName, defaultBranch).trim()}
${(BADGES[repoOwner] ?? BADGES.default)({ repoOwner, repoName, defaultBranch }).trim()}

> ${pkg.description}

Expand Down
24 changes: 11 additions & 13 deletions src/check-project/readme/license.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
/**
* @type {Record<string, (repoOwner: string, repoName: string, defaultBranch: string) => string>}
* @typedef {import('../../types').ReadmeStringGeneratorInputOptions} ReadmeStringGeneratorInputOptions
* @type {Record<string, (options: Omit<ReadmeStringGeneratorInputOptions, 'defaultBranch' | 'pkg'>) => string>}
*/
const licenses = {
ipfs: (repoOwner, repoName, defaultBranch) => `
ipfs: ({ repoUrl, repoOwner, repoName }) => `
## License

Licensed under either of

* Apache 2.0, ([LICENSE-APACHE](LICENSE-APACHE) / http://www.apache.org/licenses/LICENSE-2.0)
* MIT ([LICENSE-MIT](LICENSE-MIT) / http://opensource.org/licenses/MIT)
* Apache 2.0, ([LICENSE-APACHE](${repoUrl}/LICENSE-APACHE) / http://www.apache.org/licenses/LICENSE-2.0)
* MIT ([LICENSE-MIT](${repoUrl}/LICENSE-MIT) / http://opensource.org/licenses/MIT)

## Contribute

Expand All @@ -22,13 +23,13 @@ Unless you explicitly state otherwise, any contribution intentionally submitted

[![](https://cdn.rawgit.com/jbenet/contribute-ipfs-gif/master/img/contribute.gif)](https://github.com/ipfs/community/blob/master/CONTRIBUTING.md)
`,
default: (repoOwner, repoName, defaultBranch) => `
default: ({ repoUrl }) => `
## License

Licensed under either of

* Apache 2.0, ([LICENSE-APACHE](LICENSE-APACHE) / http://www.apache.org/licenses/LICENSE-2.0)
* MIT ([LICENSE-MIT](LICENSE-MIT) / http://opensource.org/licenses/MIT)
* Apache 2.0, ([LICENSE-APACHE](${repoUrl}/LICENSE-APACHE) / http://www.apache.org/licenses/LICENSE-2.0)
* MIT ([LICENSE-MIT](${repoUrl}/LICENSE-MIT) / http://opensource.org/licenses/MIT)

## Contribution

Expand All @@ -37,11 +38,8 @@ Unless you explicitly state otherwise, any contribution intentionally submitted
}

/**
* @param {*} pkg
* @param {string} repoOwner
* @param {string} repoName
* @param {string} defaultBranch
* @type {import('../../types').LicenseStringGenerator}
*/
export const LICENSE = (pkg, repoOwner, repoName, defaultBranch) => {
return (licenses[repoOwner] ?? licenses.default)(repoOwner, repoName, defaultBranch)
export const LICENSE = ({ repoOwner, repoName, repoUrl }) => {
return (licenses[repoOwner] ?? licenses.default)({ repoName, repoOwner, repoUrl })
}
24 changes: 23 additions & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,25 @@ interface RunOptions {
bail?: boolean
}

interface ReadmeStringGeneratorInputOptions {
repoOwner: string
repoName: string
repoUrl: string
}

interface LicenseStringGenerator {
(options: ReadmeStringGeneratorInputOptions): string
}

interface ReadmeStringGenerator {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional types.

(options: ReadmeStringGeneratorInputOptions & {
defaultBranch: string
pkg: typeof import('../package.json') & {
workspaces?: string[]
}
}): string
}

export type {
PartialOptions,
Options,
Expand All @@ -374,5 +393,8 @@ export type {
ReleaseRcOptions,
DependencyCheckOptions,
ExecOptions,
RunOptions
RunOptions,
LicenseStringGenerator,
ReadmeStringGenerator,
ReadmeStringGeneratorInputOptions
}