Chevrotain / chevrotain

Parser Building Toolkit for JavaScript
https://chevrotain.io
Apache License 2.0
2.44k stars 200 forks source link

Update parser.ts #1794

Closed kevinkhill closed 2 years ago

kevinkhill commented 2 years ago

Not exactly sure where to put this in your fancy mixin class.... but I seem to do this check a lot, so this could be a helpful little getter.

bd82 commented 2 years ago
[warn] packages/chevrotain/src/parse/parser/parser.ts
[warn] Code style issues found in the above file(s). Forgot to run Prettier?

So this seems like a trivial improvement but it is actually more complex than first seems.

  1. To update the public APIs you would also need to update this file:

  2. While this getter can be useful, I assume end users would also need to access the errors if there are any so if they have to know the .errors property anyhows, does it make sense to add another method to the API? Someone using lodash may do _.isEmpty(this.errors) and someone who likes OOP could add a method like yours o their parser class, this seems sufficient from my POV.

  3. If this new hasErrors method is an official public API, I would expect all the samples / docs to be updated as well, and that may not be worth the effort for such a small API upgrade.

kevinkhill commented 2 years ago

All great points, thanks for the detailed answer. I think I thought I could sneak in a little thing not realizing it's tendrils.

bd82 commented 2 years ago

All great points, thanks for the detailed answer. I think I thought I could sneak in a little thing not realizing it's tendrils.

😄

I'll close this PR for now. Feel free to re-open it if / when you want to go more in-depth into it.