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.

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

Ignore links to other branches #65

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion lib/find/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,12 @@ export function config(ctx) {

if (info && info.type !== 'gist') {
if (info.type in viewPaths) {
urlConfig.prefix = '/' + info.path() + '/' + viewPaths[info.type] + '/'
urlConfig.prefix =
new URL(info.browse({treepath: viewPaths[info.type]})).pathname + '/'
Copy link
Member

Choose a reason for hiding this comment

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

this looks complex, compared to the previous code, what’s browse returning, what happens new URL().pathname?

Copy link
Author

Choose a reason for hiding this comment

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

hostedGitInfo.fromUrl('https://github.com/DefinitelyTyped/DefinitelyTyped.git').browse({treepath: 'blob'})
// -> 'https://github.com/DefinitelyTyped/DefinitelyTyped'

hostedGitInfo.fromUrl('https://github.com/DefinitelyTyped/DefinitelyTyped.git#master').browse({treepath: 'blob'})
// -> 'https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master'

https://github.com/npm/hosted-git-info/blob/702b7df467f2fd4bf46395134c62534bb966ca7b/git-host-info.js#L8

so hosted-git-info omits the "blob" segment from branch-less URLs.

// GitHost#browse() omits the "blob" segment from branch-less URLs.
if (!info.committish) {
urlConfig.prefix += viewPaths[info.type] + '/'
Copy link
Member

Choose a reason for hiding this comment

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

again? viewPaths[info.type] was just used?

}
}

if (info.type in headingPrefixes) {
Expand Down
7 changes: 6 additions & 1 deletion lib/find/find-references.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,16 @@ function urlToPath(value, config, type) {

value = url.pathname.slice(config.urlConfig.prefix.length)

// Drop the first path segment (the branch) if the prefix is branch-less (matches every branch).
// Make an educated guess whether the prefix is branch-less: Currently every hosted-git-info browsetemplate/pathtemplate has <= 2 segments (`${user}/${project}`), plus e.g. "blob" and surrounding slashes -> 5.
//
// Things get interesting here: branches: `foo/bar/baz` could be `baz` on
// the `foo/bar` branch, or, `baz` in the `bar` directory on the `foo`
// branch.
// Currently, we’re ignoring this and just not supporting branches.
value = value.split(slash).slice(1).join(slash)
if (config.urlConfig.prefix.split('/').length <= 5) {
value = value.split(slash).slice(1).join(slash)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is way too magic? It doesn’t seem “educated”? Who says branches are 5+ characters?

Copy link
Author

Choose a reason for hiding this comment

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

What I was wanting to get at wasn't the characters:

'/DefinitelyTyped/DefinitelyTyped/blob/'.split('/')`
// -> [ '', 'DefinitelyTyped', 'DefinitelyTyped', 'blob', '' ]

so .length <= 5. Currently this is true of every hosted-git-info browsetemplate/pathtemplate: https://github.com/npm/hosted-git-info/blob/702b7df467f2fd4bf46395134c62534bb966ca7b/git-host-info.js#L8


return normalize(
path.resolve(config.root, value + (type === 'image' ? '' : url.hash)),
Expand Down
6 changes: 4 additions & 2 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@
* URL to hosted Git.
* If `repository` is nullish, the Git origin remote is detected.
* If the repository resolves to something npm understands as a Git host such
* as GitHub, GitLab, or Bitbucket, full URLs to that host (say,
* `https://github.com/remarkjs/remark-validate-links/readme.md#install`) can
* as GitHub, GitLab, or Bitbucket, then full URLs to that host (say,
* <https://github.com/remarkjs/remark-validate-links/blob/main/readme.md#install>) can
* also be checked.
* If it resolves to a branch (e.g. <https://github.com/remarkjs/remark-validate-links.git#main>) then links to other branches are ignored --- otherwise the plugin checks links to all branches as if they pointed to the current branch.
* When the repository is automatically detected the plugin checks links to all branches.
* If you’re not in a Git repository, you must pass `repository: false`
* explicitly.
* @property {string} [root]
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
"scripts": {
"build": "rimraf \"lib/**/*.d.ts\" \"test/**/*.d.ts\" \"*.d.ts\" && tsc && type-coverage",
"format": "remark . -qfo --ignore-pattern test/ && prettier . -w --loglevel warn && xo --fix",
"test-api": "node --conditions development test/index.js",
"test-api": "tape test/index.js test/other-branches.js",
"test-coverage": "c8 --check-coverage --branches 100 --functions 100 --lines 100 --statements 100 --reporter lcov npm run test-api",
"test": "npm run build && npm run format && npm run test-coverage"
},
Expand Down
11 changes: 9 additions & 2 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,15 @@ URL to hosted Git (`string?` or `false`).
If `repository` is nullish, the Git origin remote is detected.
If the repository resolves to something [npm understands][package-repository] as
a Git host such as GitHub, GitLab, or Bitbucket, then full URLs to that host
(say, `https://github.com/remarkjs/remark-validate-links/readme.md#install`) can
also be checked.
(say,
<https://github.com/remarkjs/remark-validate-links/blob/main/readme.md#install>)
can also be checked.
If it resolves to a branch (e.g.
<https://github.com/remarkjs/remark-validate-links.git#main>) then links to
other branches are ignored --- otherwise the plugin checks links to all branches
as if they pointed to the current branch.
When the repository is automatically detected the plugin checks links to all
branches.
If you’re not in a Git repository, you must pass `repository: false`
explicitly.

Expand Down
93 changes: 93 additions & 0 deletions test/other-branches.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import {remark} from 'remark'
import test from 'tape'
import {engine as engineCb} from 'unified-engine'
import {promisify} from 'node:util'
import {VFile} from 'vfile'
import remarkValidateLinks from '../index.js'

const engine = promisify(engineCb)

// Should ignore links to other branches.
const options = {repository: 'https://github.com/wooorm/test.git#main'}

test('same branch, working link', async (t) => {
t.deepEqual(
await process(
'[](https://github.com/wooorm/test/blob/main/examples/github.md#hello)'
),
[],
'nothing to report'
)
})

test('other branch, working link', async (t) => {
t.deepEqual(
await process(
'[](https://github.com/wooorm/test/blob/foo-bar/examples/github.md#hello)'
),
[],
'nothing to ignore'
)
})

test('same branch, no such heading', async (t) => {
t.deepEqual(
await process(
'[](https://github.com/wooorm/test/blob/main/examples/github.md#world)'
),
[
'input.md:1:1-1:70: Link to unknown heading in `examples/github.md`: `world`'
],
'should be reported'
)
})

test('other branch, no such heading', async (t) => {
t.deepEqual(
await process(
'[](https://github.com/wooorm/test/blob/foo-bar/examples/github.md#world)'
),
[],
'should be ignored'
)
})

test('same branch, no such file', async (t) => {
t.deepEqual(
await process(
'[](https://github.com/wooorm/test/blob/main/examples/world.md#hello)'
),
[
'input.md:1:1-1:69: Link to unknown file: `examples/world.md`',
'input.md:1:1-1:69: Link to unknown heading in `examples/world.md`: `hello`'
],
'should be reported'
)
})

test('other branch, no such file', async (t) => {
t.deepEqual(
await process(
'[](https://github.com/wooorm/test/blob/foo-bar/examples/world.md#hello)'
),
[],
'should be ignored'
)
})

/**
* Run the plugin on a markdown test case and return a list of stringified messages.
* unified-engine is needed to cross-reference links between files, by creating a FileSet.
*
* @param {string} value
*/
async function process(value) {
const file = new VFile({value, path: 'input.md'})
await engine({
processor: remark,
files: [file],
plugins: [[remarkValidateLinks, options]],
silent: true
})
return file.messages.map((message) => String(message))
}