Warn on invalid markdown snippet markers - #1576
Conversation
| let path = entry.path().display().to_string(); | ||
| if let Ok(contents) = fs::read_to_string(entry.path()) { | ||
| for marker in invalid_snippet_markers(&contents) { | ||
| marker_warnings.extend_from_vec(vec
There was a problem hiding this comment.
we should be able to create DiagnosticMessages directly from invalid_snippet_markers method and then rename it to something like validate_markers (that returns marker_warnings array)
|
|
||
| /// Returns true if the line is an HTML comment that looks like a snippet marker, | ||
| /// but does not parse as a supported marker. | ||
| pub fn is_invalid_snippet_marker(line: &str) -> bool { |
There was a problem hiding this comment.
just from readability perspective, is_invalid is harder to read / understand
| pub fn is_invalid_snippet_marker(line: &str) -> bool { | |
| pub fn is_valid_snippet_marker(line: &str) -> bool { |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-07 15:42 UTC Review the latest changes. Status above doesn't look right?
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1576 +/- ##
=======================================
- Coverage 82.3% 81.0% -1.3%
=======================================
Files 129 130 +1
Lines 10971 11509 +538
=======================================
+ Hits 9032 9333 +301
- Misses 1939 2176 +237 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Addressed the review comments: marker validation now returns diagnostics directly, the parser helper uses positive marker validation, and the bool::then path is gone. |
| /// contents: The contents of the markdown. | ||
| /// attribute_registry_base_url: Legacy mechanism to pass parameters to the snippet templates. | ||
| /// Returns: the updated markdown or an error. | ||
| fn update_markdown_contents( |
There was a problem hiding this comment.
It MAY be tedious - and you can add a TODO for this if so -
But we can just remove this method or update its signature to take WResult directly. I'd rather do that than have two methods here which do the same thing.
There was a problem hiding this comment.
Can you remove this method now?
It looks like it's just a passthrough in all cases.
|
Hi @ahfoysal — just a friendly reminder that this pull request is waiting on you. There are still items that need your attention. See the dashboard status comment for the full list. You don't need to push a code change to hand it back — replying to move each discussion forward is enough, whether that's answering a question, explaining why no change is needed, or asking a follow-up. The dashboard then automatically routes it back to reviewers. If you believe this pull request is incorrectly routed as waiting on the author, comment |
4 participants
Refs #501
This warns when
registry update-markdownsees an HTML comment that looks like a semconv/weaver snippet marker but does not parse as one.Marker validation now runs during the generator's existing line-by-line Markdown update pass and returns warnings as non-fatal
WResultdiagnostics, avoiding a second file read and parse. Existing whitespace-tolerant marker parsing remains unchanged.Tests:
cargo fmt --all --checkcargo test -p weaver_semconv_gen recognizes_invalid_snippet_markerscargo test -p weaver test_registry_update_markdown_warns_on_invalid_snippet_markercargo clippy -p weaver_semconv_gen --all-targets --no-deps