CharReader: Add StructuredError #1409

martinduffy1 · 2022-06-06T19:56:13Z

The move from Json::Reader to Json::CharReader omitted the ability to get structured error messages from the parser. Clients which want to extract the line and column information have to parse the returned string. Add a new getStructuredErrors() method which exposes the error as structured data instead of a string.

Based on the work from @KyleFromKitware and comments from @BillyDonahue on #1281.

ahedberg · 2022-08-15T20:19:17Z

Out of curiosity, what's the status of this PR? We'd find this addition useful as well.

BillyDonahue · 2022-08-15T22:28:45Z

Out of curiosity, what's the status of this PR? We'd find this addition useful as well.

I have some pending comments that I never hit "Send" on. Sorry about that.

src/test_lib_json/main.cpp

include/json/reader.h

src/lib_json/json_reader.cpp

martinduffy1 · 2022-10-04T19:12:23Z

@BillyDonahue I responded to all the comments from your last review. Let me know if you have any questions.

include/json/reader.h

src/lib_json/json_reader.cpp

include/json/reader.h

src/lib_json/json_reader.cpp

martinduffy1 · 2022-10-05T17:56:24Z

Thank you for the quick review @BillyDonahue
I added some comments about why I am handling impl_ the way I am, and fixed everything else according to your suggestions.

include/json/reader.h

src/lib_json/json_reader.cpp

include/json/reader.h

src/lib_json/json_reader.cpp

include/json/reader.h

martinduffy1 · 2022-10-07T15:23:20Z

@BillyDonahue thanks for your review, I've addressed these comments.

BillyDonahue · 2022-10-11T09:23:11Z

@BillyDonahue thanks for your review, I've addressed these comments.

I still see some comments unaddressed.

martinduffy1 · 2022-10-12T14:00:17Z

Which comments are unaddressed? Is the problem that I didn't click resolve on the comments? I did notice another indentation error though which I will commit now.

…

On Tue, Oct 11, 2022 at 5:23 AM Billy Donahue ***@***.***> wrote: @BillyDonahue <https://github.com/BillyDonahue> thanks for your review, I've addressed these comments. I still see some comments unaddressed. — Reply to this email directly, view it on GitHub <#1409 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZKCO2OVQIT6JHRG7X37HNDWCUWYVANCNFSM5YAQMQ4Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

BillyDonahue · 2022-10-12T21:26:42Z

Which comments are unaddressed? Is the problem that I didn't click resolve on the comments? I did notice another indentation error though which I will commit now.

Please look through some of those comments. I think you'll see what I mean.

martinduffy1 · 2022-10-13T13:31:11Z

Which comments are unaddressed? Is the problem that I didn't click resolve on the comments? I did notice another indentation error though which I will commit now.

Please look through some of those comments. I think you'll see what I mean.

The comment you responded to was addressed elsewhere in the code. I added some comments to the issues not showing as Outdated to explain how these were addressed. In the future, I'll make sure to include a description of how the comments were addressed to make it more straightforward to review.

martinduffy1 · 2022-10-21T15:23:13Z

Which comments are unaddressed? Is the problem that I didn't click resolve on the comments? I did notice another indentation error though which I will commit now.

Please look through some of those comments. I think you'll see what I mean.

The comment you responded to was addressed elsewhere in the code. I added some comments to the issues not showing as Outdated to explain how these were addressed. In the future, I'll make sure to include a description of how the comments were addressed to make it more straightforward to review.

@BillyDonahue can you take a look at this comment and let me know if there are still changes you are waiting on from me, or if you just haven't gotten a chance to review them yet?

BillyDonahue · 2022-10-25T03:22:04Z

Which comments are unaddressed? Is the problem that I didn't click resolve on the comments? I did notice another indentation error though which I will commit now.

Please look through some of those comments. I think you'll see what I mean.

The comment you responded to was addressed elsewhere in the code. I added some comments to the issues not showing as Outdated to explain how these were addressed. In the future, I'll make sure to include a description of how the comments were addressed to make it more straightforward to review.

@BillyDonahue can you take a look at this comment and let me know if there are still changes you are waiting on from me, or if you just haven't gotten a chance to review them yet?

Sorry it's very hard to follow Github PR reviews, so if something is addressed elsewhere I may not make the connection. I will take another deeper look.

src/lib_json/json_reader.cpp

BillyDonahue · 2022-11-19T19:44:42Z

src/lib_json/json_reader.cpp

@@ -900,7 +900,7 @@ class OurReader {
  bool parse(const char* beginDoc, const char* endDoc, Value& root,
             bool collectComments = true);
  String getFormattedErrorMessages() const;
-  std::vector<StructuredError> getStructuredErrors() const;
+  std::vector<OurReader::StructuredError> getStructuredErrors() const;


revert. we're in OurReader's class scope already

BillyDonahue · 2022-11-19T20:01:37Z

src/lib_json/json_reader.cpp

+    std::vector<CharReader::StructuredError> getStructuredErrors() const override {
+      std::vector<OurReader::StructuredError> errors = reader_.getStructuredErrors();
+      std::vector<CharReader::StructuredError> out_errors;
+      std::transform(errors.begin(), errors.end(), std::back_inserter(out_errors),
+        [](OurReader::StructuredError x) {
+          CharReader::StructuredError y;
+          y.offset_start = x.offset_start;
+          y.offset_limit = x.offset_limit;
+          y.message = x.message;
+          return y;
+        });
+      return out_errors;
+    }


Can be written more economically, but I think the translation can be made unnecessary.

Suggested change

std::vector<CharReader::StructuredError> getStructuredErrors() const override {

std::vector<OurReader::StructuredError> errors = reader_.getStructuredErrors();

std::vector<CharReader::StructuredError> out_errors;

std::transform(errors.begin(), errors.end(), std::back_inserter(out_errors),

[](OurReader::StructuredError x) {

CharReader::StructuredError y;

y.offset_start = x.offset_start;

y.offset_limit = x.offset_limit;

y.message = x.message;

return y;

});

return out_errors;

}

std::vector<CharReader::StructuredError> getStructuredErrors() const override {

std::vector<CharReader::StructuredError> out;

auto in = reader_.getStructuredErrors();

out.reserve(in.size());

for (auto&& x : in)

out.push_back({x.offset_start, x.offset_limit, std::move(x.message)});

return out;

}

But I don't see the need for the extra translation step.

There doesn't need to be a separate type for OurReader::StructuredError now that there's a CharReader::StructuredError. This OurReader is a private detail of this .cpp file, and its only purpose is to serve as an implementation detail of OurCharReader. It can be made to produce CharReader::StructuredError structs directly.

A great thing about OurReader is that it's 100% an implementation detail of the .cpp file and we can change it without worrying about API breaks. I didn't realize this before, which wasted your time perhaps. Apologies.
I feel like the Our stuff makes the library pretty confusing to work on and we should just make clean break from the deprecated Reader by putting it in another file where it can't bother us anymore. :)

BillyDonahue · 2022-11-19T20:03:38Z

include/json/reader.h

+      virtual std::vector<StructuredError> getStructuredErrors() const = 0;
+  };
+
+  CharReader(Impl* impl) : _impl(impl) { }


My suggestion was to accept a std::unique_ptr<Impl> here.

Unary constructors should be explicit.

Suggested change

CharReader(Impl* impl) : _impl(impl) { }

explicit CharReader(std::unique_ptr<Impl> impl) : _impl(std::move(impl)) { }

BillyDonahue · 2022-11-19T20:13:07Z

src/lib_json/json_reader.cpp

-    bool ok = reader_.parse(beginDoc, endDoc, *root, collectComments_);
-    if (errs) {
-      *errs = reader_.getFormattedErrorMessages();
+    : CharReader(new OurImpl(collectComments, features)) {}


Suggested change

: CharReader(new OurImpl(collectComments, features)) {}

: CharReader(std::make_unique<OurImpl>(collectComments, features)) {}

martinduffy1 · 2022-12-16T16:42:04Z

@BillyDonahue let me know if these changes address your comment

martinduffy1 · 2023-01-18T15:55:24Z

@BillyDonahue it's been 8 weeks since your last update on this. Is there anything you're waiting on from me?

BillyDonahue · 2023-01-18T17:52:59Z

@BillyDonahue it's been 8 weeks since your last update on this. Is there anything you're waiting on from me?

Sorry. I made a few suggestions and didn't hear back. If you have resolved them can you comment and resolve?
Update: My mistake. You responded on Dec 16 and I missed it. I'll take a look.

martinduffy1 · 2023-03-01T18:30:47Z

@BillyDonahue I see this marked as approved. Is there a release timeline that I can expect for these changes to get merged?

BillyDonahue · 2023-03-01T20:16:26Z

@BillyDonahue I see this marked as approved. Is there a release timeline that I can expect for these changes to get merged?

Sorry I thought the approval was sufficient for you to merge it rather than me.
I see that we have to rerun Travis.

BillyDonahue · 2023-03-01T20:28:11Z

Hm. I don't actually know how to restart the stuck Travis CI job.
I poked around and didn't find anything.
If you push an update maybe it will kickstart itself.

Add getStructuredError to CharReader

martinduffy1 · 2023-03-02T15:05:59Z

@BillyDonahue I squashed the commits and pushed to try to retrigger the CI job. I don't think I can merge this as I don't have write access to the repository. I'm also wondering if there can be a minor version increase when this and #1412 are merged so that we can detect versions with these updates.

martinduffy1 mentioned this pull request Jul 15, 2022

Expose getLocationLineAndColumn() as a public API #1412

Closed

martinduffy1 force-pushed the errorinfo branch from 0a692b3 to aab0cc8 Compare July 21, 2022 17:06

BillyDonahue requested changes Aug 15, 2022

View reviewed changes

martinduffy1 force-pushed the errorinfo branch from 90c3080 to 59780d8 Compare October 4, 2022 19:10

BillyDonahue requested changes Oct 5, 2022

View reviewed changes

src/lib_json/json_reader.cpp Outdated Show resolved Hide resolved

src/lib_json/json_reader.cpp Outdated Show resolved Hide resolved

martinduffy1 force-pushed the errorinfo branch from 28477f4 to 4c0a5ef Compare October 5, 2022 17:54

BillyDonahue requested changes Oct 6, 2022

View reviewed changes

martinduffy1 force-pushed the errorinfo branch from fedff8c to e9e3306 Compare October 6, 2022 19:11

martinduffy1 force-pushed the errorinfo branch from f9ebf1f to 63a42d7 Compare October 12, 2022 14:00

BillyDonahue closed this Oct 12, 2022

BillyDonahue reopened this Oct 12, 2022

BillyDonahue reviewed Oct 25, 2022

View reviewed changes

src/lib_json/json_reader.cpp Outdated Show resolved Hide resolved

src/lib_json/json_reader.cpp Outdated Show resolved Hide resolved

martinduffy1 force-pushed the errorinfo branch from 63a42d7 to 804d8f0 Compare November 4, 2022 17:51

BillyDonahue requested changes Nov 20, 2022

View reviewed changes

martinduffy1 force-pushed the errorinfo branch from 6ccfec6 to 0056697 Compare November 25, 2022 13:12

BillyDonahue approved these changes Jan 18, 2023

View reviewed changes

CharReader: Add Structured Error

9f3b2ef

Add getStructuredError to CharReader

martinduffy1 force-pushed the errorinfo branch from 78f6dd6 to 9f3b2ef Compare March 2, 2023 15:03

baylesj and others added 2 commits September 9, 2024 18:44

Merge branch 'master' into errorinfo

7f7b02b

run clang format

2ff6cbc

baylesj merged commit c04c0c2 into open-source-parsers:master Sep 10, 2024
7 checks passed

baylesj mentioned this pull request Sep 12, 2024

CharReader: Add StructuredError #1281

Closed

chenrui333 mentioned this pull request Sep 13, 2024

jsoncpp 1.9.6 Homebrew/homebrew-core#189352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CharReader: Add StructuredError #1409

CharReader: Add StructuredError #1409

martinduffy1 commented Jun 6, 2022

ahedberg commented Aug 15, 2022

BillyDonahue commented Aug 15, 2022

martinduffy1 commented Oct 4, 2022

martinduffy1 commented Oct 5, 2022

martinduffy1 commented Oct 7, 2022

BillyDonahue commented Oct 11, 2022

martinduffy1 commented Oct 12, 2022 via email

BillyDonahue commented Oct 12, 2022 •

edited

Loading

martinduffy1 commented Oct 13, 2022

martinduffy1 commented Oct 21, 2022

BillyDonahue commented Oct 25, 2022

BillyDonahue Nov 19, 2022

BillyDonahue Nov 19, 2022

BillyDonahue Nov 19, 2022

BillyDonahue Nov 19, 2022

martinduffy1 commented Dec 16, 2022

martinduffy1 commented Jan 18, 2023

BillyDonahue commented Jan 18, 2023 •

edited

Loading

martinduffy1 commented Mar 1, 2023

BillyDonahue commented Mar 1, 2023

BillyDonahue commented Mar 1, 2023

martinduffy1 commented Mar 2, 2023

	CharReader(Impl* impl) : _impl(impl) { }
	explicit CharReader(std::unique_ptr<Impl> impl) : _impl(std::move(impl)) { }

	: CharReader(new OurImpl(collectComments, features)) {}
	: CharReader(std::make_unique<OurImpl>(collectComments, features)) {}

CharReader: Add StructuredError #1409

CharReader: Add StructuredError #1409

Conversation

martinduffy1 commented Jun 6, 2022

ahedberg commented Aug 15, 2022

BillyDonahue commented Aug 15, 2022

martinduffy1 commented Oct 4, 2022

martinduffy1 commented Oct 5, 2022

martinduffy1 commented Oct 7, 2022

BillyDonahue commented Oct 11, 2022

martinduffy1 commented Oct 12, 2022 via email

BillyDonahue commented Oct 12, 2022 • edited Loading

martinduffy1 commented Oct 13, 2022

martinduffy1 commented Oct 21, 2022

BillyDonahue commented Oct 25, 2022

BillyDonahue Nov 19, 2022

Choose a reason for hiding this comment

BillyDonahue Nov 19, 2022

Choose a reason for hiding this comment

BillyDonahue Nov 19, 2022

Choose a reason for hiding this comment

BillyDonahue Nov 19, 2022

Choose a reason for hiding this comment

martinduffy1 commented Dec 16, 2022

martinduffy1 commented Jan 18, 2023

BillyDonahue commented Jan 18, 2023 • edited Loading

martinduffy1 commented Mar 1, 2023

BillyDonahue commented Mar 1, 2023

BillyDonahue commented Mar 1, 2023

martinduffy1 commented Mar 2, 2023

BillyDonahue commented Oct 12, 2022 •

edited

Loading

BillyDonahue commented Jan 18, 2023 •

edited

Loading