Record parse failure reason and location #252
Conversation
|
Happy to discuss alternatives to this, this was just the simplest approach that came to mind and I figured it's easier to make a code-based suggestion than an abstract one. |
Yep. |
|
@LeszekSwirski Can you sync your fork with our main branch. I had to do a forced update to fix problems I created earlier today. Your PR seems quite reasonable and maybe we want to include it in a release very soon. |
LeszekSwirski
force-pushed
the
parse-error
branch
from
812dd07
to
21cec71
Compare
|
Sure, done. |
|
I will push a patch release immediately because I want your fix from an earlier PR. Meanwhile let us turn this PR into at least a minor release. |
|
Sounds reasonable, it's an observable API change if anyone checks |
|
@deadalnix Would you have a look? I think that this PR is a nice step up. |
|
@LeszekSwirski I am not 100% sure that we have users of this part of our API. But it is more than a patch for sure. |
|
The obvious thing that jump to my eyes here is the lack of tests :) The feature looks good, the implementation fairly uncontroversial considering the conditions are already checked for, so for me, as long as there is a good test, I think it should go it. |
|
Fair comment 😄 I'll add some tests tomorrow. |
LeszekSwirski
force-pushed
the
parse-error
branch
from
21cec71
to
dd2dcb8
Compare
|
@LeszekSwirski Thanks for adding tests. |
tests/json_fmt.cpp
Outdated
| const std::string input = "inf"; // not valid in JSON | ||
| double result; | ||
| fast_float::parse_options options{ fast_float::chars_format::json }; | ||
| auto answer = fast_float::from_chars_advanced(input.data(), input.data()+input.size(), result, options); | ||
| if(answer.ec == std::errc()) { std::cerr << "should have failed\n"; return EXIT_FAILURE; } | ||
| auto answer = fast_float::from_chars_advanced( | ||
| input.data(), input.data() + input.size(), result, options); | ||
| if (answer.ec == std::errc()) { | ||
| std::cerr << "should have failed\n"; | ||
| return EXIT_FAILURE; | ||
| } | ||
| return EXIT_SUCCESS; |
There was a problem hiding this comment.
You have indentation problems here.
@lemire what do you think about adding a clang-format config file so we can avoid this type of review cycles?
There was a problem hiding this comment.
@deadalnix Please review #257
I don't think we want to consider formatting mistakes as blocking for a PR. We can reformat later.
|
This PR looks good to me. I am inviting further review. This would be at least a minor release. |
|
Sorry, the formatting was a bad push, it wasn't intended for review |
In parse_number_string, if there is a parse error, report the specific error as one of the values in a new parse_error enum, and update lastmatch to match the error location. This allows users of the library to print more helpful error messages for invalid inputs.
LeszekSwirski
force-pushed
the
parse-error
branch
from
dd2dcb8
to
b6ce2c4
Compare
|
@deadalnix ok now the formatting is fixed (just a "format on save" mess up) |
Thankfully, we have tools to automatically format. Although, for this project, we don't have a standard yet. If you are interested, I have a PR outstanding that would prescribe a given style: |
|
The tests pass. We will merge this PR unless someone objects in the near future. |
|
Merging. This will be part of the next release. |
In parse_number_string, if there is a parse error, report the specific
error as one of the values in a new parse_error enum, and update
lastmatch to match the error location. This allows users of the library
to print more helpful error messages for invalid inputs.