-
Notifications
You must be signed in to change notification settings - Fork 29
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
Create a more flexible HTML comment parser and raise warning #516
base: main
Are you sure you want to change the base?
Conversation
let (input, result) = take_until("-->")(input)?; | ||
|
||
// Check for spacing issues and warn if found | ||
if result.starts_with("semconv") && !input.trim().starts_with("semconv ") { |
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.
Three things:
-
I don't think we need to force spaces here.
-
For the purpose of separation of concerns, the validation logic should go in
parse_semconv_snippet_directive
. Ideally this function JUST parses HTML headers, which have no restrictions on whitespaces. -
We're not using
eprintln!
for warnings, instead you'll want to find a way to return aDiagnostic
, e.g. using weaver's result WResult. This might require some wiring to make sure non-fatal-errors (NFE) can make it out of the parser. I haven't tried to sort out NFE w/ the nom parsers bits yet. Let me know if you have issues I'll see if I can put a scaffold up for you there.
A quick thought on warnings here:
if parser::is_markdown_snippet_directive(line) {
... existing ...
} else if parser::is_possible_markdown_snippet_directive(line) {
... issue warnings ...
} Where |
Towards: #501
This PR builds on top of the work from here. Create a more flexible HTML comment parser so we don't rely on spaces to exist to succesfully parse HTML comments. Also, raise warnings when there are missing spaces.