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

[linters] Check link-anchor for # stutter #2313

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Feb 3, 2022

This adds a link-anchor hash-stutter check for an error that seems to creep in now and again, for example:

NOTE: The build for this PR will fail (demonstrating that the check is working) until #2312 is merged.

/cc @austinlparker @tedsuo @carlosalberto

@chalin chalin requested review from a team February 3, 2022 13:43
@chalin
Copy link
Contributor Author

chalin commented Feb 3, 2022

Here is an example of the check at work (excerpt from https://github.com/open-telemetry/opentelemetry-specification/runs/5052423559?check_suite_focus=true):

$ make markdown-link-check
FILE: ./specification/compatibility/opentracing.md

  37 links checked.

Error: link anchor contains double ## on these line(s):
504:[Link](../trace/api.md##specifying-links) functionality. This information
make: *** [Makefile:33: markdown-link-check] Error 1

Error: Process completed with exit code 2.

@chalin
Copy link
Contributor Author

chalin commented Feb 3, 2022

Of course this is a simple syntactic check that might, down the line, register false-positives but IMHO we can face that situation if/when it arises. In the meantime, it'll be nice to catch anchor-hash stutters on PR submission.

@chalin chalin force-pushed the chalin-check-link-anchor-stutter-2022-02-03 branch from 8c0e51a to 248ded7 Compare February 3, 2022 15:01
@chalin
Copy link
Contributor Author

chalin commented Feb 3, 2022

#2312 has been merged, I've rebased. The check is still failing -- I'm investigating now.

@chalin chalin marked this pull request as draft February 3, 2022 15:11
@chalin chalin marked this pull request as ready for review February 3, 2022 15:45
@@ -29,7 +30,13 @@ $(MARKDOWN_LINK_CHECK):

.PHONY: markdown-link-check
markdown-link-check: $(MARKDOWN_LINK_CHECK)
@for f in $(ALL_DOCS); do $(MARKDOWN_LINK_CHECK) --quiet --config .markdown_link_check_config.json $$f; done
@for f in $(ALL_DOCS); do \
$(MARKDOWN_LINK_CHECK) --quiet --config .markdown_link_check_config.json $$f; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so this is the current behavior:

  • If there is a double hash in an anchor, the check will fail
  • If there are invalid links, they will be reported by markdown-link-check, but the check will pass. This was the behavior before this PR -- presumably because there were too many false positives(?). If this was an oversight and you'd like the check to fail when invalid links are discovered, then all we need to do is change the ; at the end of this line to &&.

Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

I think you found a bug here. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chalin chalin changed the title [checks] Check link-anchor for # stutter [editorial] Check link-anchor for # stutter Feb 7, 2022
@chalin chalin force-pushed the chalin-check-link-anchor-stutter-2022-02-03 branch 2 times, most recently from be1330b to 6c4edff Compare February 15, 2022 19:37
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 23, 2022
@chalin chalin force-pushed the chalin-check-link-anchor-stutter-2022-02-03 branch from 6c4edff to f3fa33d Compare February 23, 2022 15:34
@chalin
Copy link
Contributor Author

chalin commented Feb 23, 2022

@carlosalberto et all: any consideration being given to this extra check?

@github-actions github-actions bot removed the Stale label Feb 24, 2022
@github-actions
Copy link

github-actions bot commented Mar 3, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 3, 2022
@chalin chalin changed the title [editorial] Check link-anchor for # stutter [linters] Check link-anchor for # stutter Mar 3, 2022
@chalin
Copy link
Contributor Author

chalin commented Mar 4, 2022

Hi @carlosalberto et all: before I spend time resolving conflicts on this PR I wanted to know if you saw value in this extra check? If not, we can continue to detect it in the same way I've been doing it before, that is, when I build the spec through the website. I'm ok either way.

@github-actions github-actions bot removed the Stale label Mar 5, 2022
@carlosalberto
Copy link
Contributor

Hey @chalin

If this was an oversight and you'd like the check to fail when invalid links are discovered, then all we need to do is change the ; at the end of this line to &&.

As @Oberon00 mentioned, I think this was a bug.

(Sorry for the delayed answer!)

@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2022

Thanks for the feedback @carlosalberto and @Oberon00.
So do you want me to submit a separate fix, or simply update (resolve the conflicts) and resubmit this PR?

@carlosalberto
Copy link
Contributor

Updating and re-submitting it is enough with me ;)

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 18, 2022
@chalin
Copy link
Contributor Author

chalin commented Mar 23, 2022

Since the identified issue was fixed in #2323, I'm going to close this PR since I no longer thing that it is worth the extra check of anchor-hash stutters.

@chalin chalin closed this Mar 23, 2022
@chalin chalin deleted the chalin-check-link-anchor-stutter-2022-02-03 branch March 23, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants