-
Notifications
You must be signed in to change notification settings - Fork 466
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
Implement a PARSERS setting #603
Implement a PARSERS setting #603
Conversation
Codecov Report
@@ Coverage Diff @@
## master #603 +/- ##
==========================================
+ Coverage 95.12% 95.21% +0.08%
==========================================
Files 302 302
Lines 2504 2506 +2
==========================================
+ Hits 2382 2386 +4
+ Misses 122 120 -2
Continue to review full report at Codecov.
|
I think it is a really good idea. Moreover, it is necessary to implement this in some way to allow showing what parser has been used when implementing the |
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.
Really good job here.
Maybe it's not necessary to add it before merging, but it could be a good idea to mention the existing "parsers" in the docs explaining what each one does.
Related to this are the parsers names. I think we should meditate on the names as then are going to be a “public API”. Adding the explanation to the docs will help us to find out the best name for each parser.
Added the list of parsers to the documentation with a brief description about each one of them. |
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.
I left some comments. Really good job here, it could be amazing to include this in the next release (expecting to be this month).
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.
Ok, I like the approach.
I would mention the "settings" in the error, to make easier to find and fix the error, but it's not necessary, it works as expected.
Fixes #312, fixes #525