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

Handle parsing errors/warnings #37

Closed
chrisjsewell opened this issue Sep 27, 2020 · 4 comments · Fixed by #61
Closed

Handle parsing errors/warnings #37

chrisjsewell opened this issue Sep 27, 2020 · 4 comments · Fixed by #61

Comments

@chrisjsewell
Copy link
Member

Currently, exceptions in code formatting plugins are silently ignored.
I think this is a bit "dangerous" in that, if a plugin is faulty, people won't realise that their files are not being formatted as intended.

At a minimum, I think the errors should be logged, as I have added in #36.
But perhaps also it should be the default or an option (using #35) to fail on such exceptions.

Another place where reporting/failing should be considered, is when duplicate_refs are found in the env (after parsing)

@hukkin
Copy link
Member

hukkin commented Sep 27, 2020

I was also unsure (and still am) what the best default here is. I made mdformat swallow code formatter exceptions for now, since I feel like it might be quite common to write broken code in Markdown code blocks, e.g.

here's broken json in a markdown code block
```json
{
  "item1": 1,
  "item2": 2,
  ...
}
```

I'm not sure if this assumption is valid though, and for my own use, I'd be happy with mdformat returning a non-zero error code if a code formatter raises. If there's two schools of thought here I'd happily merge a CLI arg to change this behavior.

At a minimum, I think the errors should be logged

Agreed.

Another place where reporting/failing should be considered, is when duplicate_refs are found in the env (after parsing)

What do you mean by duplicate_refs? Is this something that happens (and should error) in markdown-it-py perhaps?

@chrisjsewell
Copy link
Member Author

yeh good point about writing broken code, I think then failure should be optional via a CLI arg 👍

What do you mean by duplicate_refs?

See https://github.com/executablebooks/markdown-it-py/blob/c3247296415c69cb3083376a78c56e4da8249815/markdown_it/rules_block/reference.py#L197, if you write:

[a]: href1
[a]: href2 

The 2nd one will be stored in duplicate_refs.
It is the renderers job to decide if it/how they want to act on these duplicates (for example in myst-parser a warning is sent to the Sphinx error reporter)

@hukkin
Copy link
Member

hukkin commented Sep 27, 2020

Ah gotcha.

I'm now thinking since mdformat is a formatter, not a validator, if there is a problem that can't be fixed by formatting, then the exit code should probably by default always be success (this goes for "invalid" code block contents). We could consider printing a warning to stderr though.

When it comes to duplicate_refs, if i understand correctly, that is something that mdformat can fix, by simply only rendering the first ref (with the assumption that rendering ref links is supported, currently isn't). So I feel like no warnings or anything should be needed.

@chrisjsewell
Copy link
Member Author

Yeh I agree these things should probably default to success, but it would be nice to (optionally) get some kind of warning.

maybe make this a bit less "hardcoded" than directly printing to stderr, by doing something like:

        try:
            code_block = fmt_func(code_block, info_str)
        except Exception:
            # Swallow exceptions so that formatter errors (e.g. due to
            # invalid code) do not crash mdformat.
            options["mdformat_logger"].warn("some message")

this is probably not the ideal solution, but you get the idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants