-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Make json_writer a structured_writer, improve output formatting #3227
Conversation
long long int is used in CmdStan
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.
Few small changes
src/stan/callbacks/json_writer.hpp
Outdated
template <typename T> | ||
void write_int_like(const std::string& key, T value) { | ||
if (output_ == nullptr) | ||
return; | ||
write_sep(); | ||
write_key(key); | ||
*output_ << value; | ||
} |
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 would just call this write_
since it takes anything that is valid for operator<<
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.
Yes, but I want to make it clear that things like double
(which are valid for operator<<
) should not be passed to this.
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.
Oh for NA etc.
I'd still call it write_
and have the signature like
template <typename T, stan::require_integral_t<T>* = nullptr>
void write_int_like(const std::string& key, T value)
src/stan/callbacks/json_writer.hpp
Outdated
if (output_ == nullptr) | ||
return; |
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.
if (output_ == nullptr) | |
return; | |
if (output_ == nullptr) { | |
return; | |
} |
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.
Done for all the functions
*output_ << "[ "; | ||
if (v.size() > 0) { | ||
auto last = v.end(); | ||
--last; | ||
for (auto it = v.begin(); it != last; ++it) { | ||
*output_ << *it << ", "; | ||
} | ||
} | ||
*output_ << v.back() << " ]"; |
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.
Same code is replicated across multiple functions. Is there a reason you got rid of write_vector
?
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.
The value type is printed differently in each case - this one uses <<
, the vector uses process_string()
, etc.
Write_vector was also just four functions which were the same as these but called from a template. To use this as a derived class we needed un-templated versions
void write(const std::string& key, | ||
const std::vector<std::complex<double>>& v) { |
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.
Need to make these virtual in structured_writer
?
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.
Looks good besides the int thing which I'll leave as optional
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Follow up on #3191.
This PR:
structured_writer
andjson_writer
such that the latter can be a subclass of the former.j.begin_record("foo"); j.end_record(); j.write("anything here");
would generate invalid JSONIntended Effect
How to Verify
Side Effects
Documentation
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: