Skip to content

Conversation

@lmolkova
Copy link
Member

@lmolkova lmolkova commented May 6, 2024

Fixes #987

Changes

Docfx was introduced in the spec repo a while ago - open-telemetry/opentelemetry-specification#742
with the intention to detect broken links.

Docfx v3 we used since open-telemetry/opentelemetry-specification#2285 was in preview and is no longer accessible and not recommended to be used even if built from sources.

Docfx is a site-generator, not a linting tool, there is no (?) documentation on checks against docs it does.

We already have individual markdown lint and link checks.

With all of this I suggest removing docfx since it seems to be unnecessary.

Merge requirement checklist

PS: I'll send a similar PR to the spec.

@lmolkova lmolkova requested review from a team May 6, 2024 21:29
@lmolkova
Copy link
Member Author

lmolkova commented May 6, 2024

/cc @reyang in case you have any context on docfx checks we might still need

@reyang
Copy link
Member

reyang commented May 6, 2024

/cc @reyang in case you have any context on docfx checks we might still need

IIRC, relative links in markdown is verified by docfx, probably something that can be covered or already covered by markdownlint.

@lmolkova
Copy link
Member Author

lmolkova commented May 6, 2024

IIRC, relative links in markdown is verified by docfx, probably something that can be covered or already covered by markdownlint.

thanks! let me check

@lmolkova lmolkova requested a review from a team May 6, 2024 21:36
@lmolkova lmolkova marked this pull request as draft May 6, 2024 21:43
@lmolkova
Copy link
Member Author

lmolkova commented May 6, 2024

thanks @reyang for the pointer, neither markdownlint nor markdown-link-check check fragment part in a different file ([see foo](./bar#baz)) and there seem to be no other common checker for it.
I'll try to resurrect docfx v2 then

@lmolkova lmolkova closed this May 6, 2024
@reyang
Copy link
Member

reyang commented May 6, 2024

FYI - https://github.com/theoludwig/markdownlint-rule-relative-links seems promising (although the download number seems low). Probably give it a try as it seems to be blessed by the owner of markdownlint DavidAnson/markdownlint#586 (comment).

@lmolkova
Copy link
Member Author

lmolkova commented May 7, 2024

you sent me to the rabbit hole @reyang 😅

apparently markdownlint-rule-relative-links does not support absolute links (e.g. /docs/http/http-spans.md#something). It does support validation for ./docs/http/http-spans.md#something.
If my JavaScript was any good, I'd consider contributing something.

The TL;DR:

docfx v2 does not support checking anchors in other files (only within the same file), which is already covered by markdownlint.

I'll merge this PR and will create an issue to figure out how to do cross-file anchor checks.

@lmolkova lmolkova reopened this May 7, 2024
@lmolkova lmolkova marked this pull request as ready for review May 7, 2024 00:50
@trask
Copy link
Member

trask commented May 7, 2024

thanks @reyang for the pointer, neither markdownlint nor markdown-link-check check fragment part in a different file ([see foo](./bar#baz)) and there seem to be no other common checker for it. I'll try to resurrect docfx v2 then

in Java, we're pinning an older version of markdown-link-check (v3.11.2) before anchor link checking broke, looks like a couple other otel repos have linked to that markdown-link-check issue too 😅

@lmolkova
Copy link
Member Author

lmolkova commented May 7, 2024

thanks @reyang for the pointer, neither markdownlint nor markdown-link-check check fragment part in a different file ([see foo](./bar#baz)) and there seem to be no other common checker for it. I'll try to resurrect docfx v2 then

in Java, we're pinning an older version of markdown-link-check (v3.11.2) before anchor link checking broke, looks like a couple other otel repos have linked to that markdown-link-check issue too 😅

cool, I'll check and send the PR - I have some other changes for that thing staged.

@lmolkova lmolkova merged commit 519bf7d into open-telemetry:main May 7, 2024
@lmolkova
Copy link
Member Author

lmolkova commented May 7, 2024

@trask I checked, it's a slightly different problem. The thing we're missing is #1009 and markdown-link-check never supported it :(

@trask
Copy link
Member

trask commented May 7, 2024

oh no, I thought we (Java) were getting cross-file anchor link checking, but did a quick test and we're not 😢

carlosalberto pushed a commit to open-telemetry/opentelemetry-specification that referenced this pull request May 7, 2024
DocFx v3 is no longer available and not intended to be used externally.

The feature it provided that we don't have from other tooling is
cross-file anchor reference check, see
open-telemetry/semantic-conventions#1009 for
the details. It seems there is no popular (or even not popular)
replacement for it.

DocFx v2 only check anchors within the same file which markdownlint does
as well.

Related:
open-telemetry/semantic-conventions#1008

Closes #4026.
drewby pushed a commit to drewby/otel-semantic-conventions that referenced this pull request May 23, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
DocFx v3 is no longer available and not intended to be used externally.

The feature it provided that we don't have from other tooling is
cross-file anchor reference check, see
open-telemetry/semantic-conventions#1009 for
the details. It seems there is no popular (or even not popular)
replacement for it.

DocFx v2 only check anchors within the same file which markdownlint does
as well.

Related:
open-telemetry/semantic-conventions#1008

Closes open-telemetry#4026.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[CI] All PRs fail on docfx check

5 participants