-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
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.
Hey!
This looks rather verbose, and it’s unclear to me what’s going on.
I’m not sure about the original request on DT. It’s not reasonable to expect this project to look across branches, but given that branches are supposed to be merged, I think it’s fine to expect that files also exist on different branches?
I may be open to this change, but not sure.
Before this PR, urlConfig.prefix is /DefinitelyTyped/DefinitelyTyped/blob/, which matches every branch/commit.
Yeah. That behavior is kinda intentional:
// Currently, we’re ignoring this and just not supporting branches. |
@@ -46,7 +46,11 @@ 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 + '/' |
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 looks complex, compared to the previous code, what’s browse
returning, what happens new URL().pathname
?
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.
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'
so hosted-git-info omits the "blob" segment from branch-less URLs.
urlConfig.prefix = | ||
new URL(info.browse({treepath: viewPaths[info.type]})).pathname + '/' | ||
if (!info.committish) { | ||
urlConfig.prefix += viewPaths[info.type] + '/' |
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.
again? viewPaths[info.type]
was just used?
value = value.split(slash).slice(1).join(slash) | ||
if (config.urlConfig.prefix.split('/').length <= 5) { | ||
value = value.split(slash).slice(1).join(slash) | ||
} |
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 is way too magic? It doesn’t seem “educated”? Who says branches are 5+ characters?
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.
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
test/index.js
Outdated
) | ||
t.deepEqual( | ||
strip(stderr), | ||
[ |
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 is a rather verbose test. What’s okay, and should (not) be okay? What’s broken and should (not) be broken?
3277409
to
933152c
Compare
I’m still not 100% on whether this is a good idea. But, particularly, I find it unclear what the implications are, because: What happens if a SHA is passed instead of a branch? What about common workflows:
|
I see what you mean: I've now added docs covering what happens when the In that workflow ☝️:
So in every case, local links are checked. |
In the latest commit I split up the test into individual test cases. |
Initial checklist
Description of changes
Append
GitHost.committish
->urlConfig.prefix
, thereby only matching links to the configured branch and ignoring others.Fixes DefinitelyTyped/DefinitelyTyped#58598
In this instance ☝️, the falsely-failing link was https://github.com/DefinitelyTyped/DefinitelyTyped/blob/1253faabf5e0d2c5470db6ea87795d7f96fef7e2/types/history/v2/tsconfig.json (it's valid).
The linked file exists in that commit but not in the main branch.
Before this PR,
urlConfig.prefix
is/DefinitelyTyped/DefinitelyTyped/blob/
, which matches every branch/commit. After, it's/DefinitelyTyped/DefinitelyTyped/blob/master/
, which only matches the configured branch.