remark-validate-links icon indicating copy to clipboard operation
remark-validate-links copied to clipboard

Ignore links to other branches

Open jablko opened this issue 3 years ago • 5 comments

Initial checklist

  • [x] I read the support docs
  • [x] I read the contributing guide
  • [x] I agree to follow the code of conduct
  • [x] I searched issues and couldn’t find anything (or linked relevant results below)
  • [x] If applicable, I’ve added docs and tests

Description of changes

Append GitHost.committish -> urlConfig.prefix, thereby only matching links to the configured branch and ignoring others.

Fixes https://github.com/DefinitelyTyped/DefinitelyTyped/issues/58598

In this instance :point_up:, 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.

jablko avatar Feb 04 '22 20:02 jablko

Hi! It seems some of the things asked in the template are missing? Please edit your post to fill out everything.

  • [ ] Initial checklist (todo)
  • [x] Description of changes

You won’t get any more notifications from me, but I’ll keep on updating this comment, and remove it when done!

If you need it, here’s the original template
<!--
  Please check the needed checkboxes ([ ] -> [x]). Leave the
  comments as they are, they won’t show on GitHub.
  We are excited about pull requests, but please try to limit the scope, provide
  a general description of the changes, and remember, it’s up to you to convince
  us to land it.
-->

### Initial checklist

*   [ ] I read the support docs <!-- https://github.com/remarkjs/.github/blob/main/support.md -->
*   [ ] I read the contributing guide <!-- https://github.com/remarkjs/.github/blob/main/contributing.md -->
*   [ ] I agree to follow the code of conduct <!-- https://github.com/remarkjs/.github/blob/main/code-of-conduct.md -->
*   [ ] I searched issues and couldn’t find anything (or linked relevant results below) <!-- https://github.com/search?q=user%3Aremarkjs&type=Issues -->
*   [ ] If applicable, I’ve added docs and tests

### Description of changes

TODO

<!--do not edit: pr-->

Thanks, — bb

github-actions[bot] avatar Feb 04 '22 20:02 github-actions[bot]

Codecov Report

Merging #65 (0937e33) into main (f259b30) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          877       890   +13     
=========================================
+ Hits           877       890   +13     
Impacted Files Coverage Δ
lib/find/config.js 100.00% <100.00%> (ø)
lib/find/find-references.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f259b30...0937e33. Read the comment docs.

codecov-commenter avatar Feb 06 '22 18:02 codecov-commenter

I’m still not 100% on whether this is a good idea.

But, particularly, I find it unclear what the implications are, because: a) to enable this feature, a “commitish” has to explicitly be passed, and there’s only one test for that. b) there are no docs

What happens if a SHA is passed instead of a branch? What if the branch was automatically detected (like the repo)?

What about common workflows:

  • I’m on the main branch and everything’s tested
  • I check out my-feature-branch to do some work
  • I refactor a bunch of things, but none of the local links are checked, because none of them go to the branch my-feature-branch?
  • The branch is merged, tons of errors occur — seems quite unfortunate?

wooorm avatar Feb 10 '22 10:02 wooorm

I see what you mean: I've now added docs covering what happens when the repository option does/doesn't have a branch, and what happens when the repo is automatically detected.

In that workflow :point_up::

  • If the repository option has a branch (e.g. https://github.com/remarkjs/remark-validate-links.git#main) the plugin checks links to the main branch, regardless of the current branch (my-feature-branch or main).

    Without this PR it doesn't check anything, because currently urlConfig.prefix is /remarkjs/remark-validate-links#main/blob/ which doesn't match any links.

  • If it doesn't have a branch the plugin checks links to all branches as if they pointed to the current branch (same behavior as today).

  • The same happens when the repo is automatically detected (same as today).

So in every case, local links are checked.

jablko avatar Feb 16 '22 17:02 jablko

In the latest commit I split up the test into individual test cases.

jablko avatar Feb 17 '22 00:02 jablko