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

Incorporate error improvements from/for YAML Doctor #493

Closed
Mr0grog opened this issue May 3, 2019 · 4 comments
Closed

Incorporate error improvements from/for YAML Doctor #493

Mr0grog opened this issue May 3, 2019 · 4 comments

Comments

@Mr0grog
Copy link

Mr0grog commented May 3, 2019

Hi, I recently did some consulting work for Asana, where I wound up implementing some tools to provide clearer error messaging and advice when people write invalid YAML code. They’ve been generous in letting me open-source it as “yaml-doctor”: https://github.com/Mr0grog/yaml-doctor

I’m sure not everything in yaml-doctor is really relevant to js-yaml, but:

  1. Some things it does are pretty simple (e.g. using nicer language and adding a bit more information to the “deficient indentation” warning), and it would be nice to incorporate them directly into js-yaml.

    Would some of these changes be welcome?

  2. Other things it does are a lot more complicated or even speculative (e.g. handling anchors that look they were meant to be HTML entities at the start of a scalar), so I imagine you wouldn’t want them in the parser.

  3. Many things rely on the listener callback, which I know isn’t totally stable, and would be aided by potentially having better hooks into the parser (e.g. an opportunity to intercede before an error, or rewind to the start of the last token, or more information about the current parsing context).

    Would some design ideas (and work on implementation) for this be welcome?

I’m more than happy to work on these things, but I wanted to check in with any maintainers or active contributors here before showing up with a bunch of random or even unwelcome PRs. :)

@puzrin
Copy link
Member

puzrin commented May 4, 2019

Things like syntax coloring and linter is class of task, which require AST. Current implementation does not have it (listener callback is dirty hack). So, i suspect, that trying to incorporate some improvements will be very hacky.

Ideally, this package need full rewrite, more close to reference libyaml implementation (with "events" layer). But with keeping in mind some JS practices to keep speed high. I have no resources for such rewrite, but all "ideas" are collected in #331.


In short: i'm ok to accept any non-hacky improvements. But you need to decide first, how many time do you wish to spend (improve v3 a bit, or do v4).

@Mr0grog
Copy link
Author

Mr0grog commented May 6, 2019

OK, sounds like I should start with just doing some simple things like better wording for some errors if there might be a big rewrite in the future.

Things like syntax coloring and linter is class of task, which require AST.

FWIW, YAML Doctor focuses on better parser errors and enabling the parser to go farther and find more errors before totally failing, so an AST wouldn’t necessarily help it. Most of what I’m doing is only applicable if js-yaml would have failed to create an AST in the first place. YAML Doctor isn’t really a linter in the traditional sense.

@puzrin
Copy link
Member

puzrin commented May 6, 2019

I'd suggest to prepare PR with 1 simple change, and i can say exactly then, if those can be merged or not. If everything ok - then you could add the rest there.

@puzrin
Copy link
Member

puzrin commented Nov 28, 2020

Close as outdated (due inactivity)

@puzrin puzrin closed this as completed Nov 28, 2020
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

No branches or pull requests

2 participants