WIP/RFC: Flexible pretty printing formatting #1121
Conversation
astyle reformatted some stuff... I... don't know why.
Just copy unit-serialization to unit-fancy-serialization
1. Copy header and rename; add to Makefile; amalgamate 2. Add as friend to basic_json 3. Copy operator<< to 'fancy_dump' and simplify 4. Change test to refer to that
This makes the depth independent of the indent_step and independent of whether it's indenting at all
Prettyfies the test cases a bit, though there is a utiliity function needed now.
Elides objects with ... if that limit is exceeded
This will be able to provide multiple styles depending on context
This is half a bug fix half just continuing work from befor
This isn't as clean as I wanted -- there's a lot more context than I realized -- but I think it's still an improvement and will become even moreso later
Instead of depending on indent_step
I really really wanted to name these the same and overload them,
but I couldn't get the metaprogramming to work right. Here's a
comment I wrote that describes the problems and what I *planned*
to do:
// What we want is the register_style overloads below. I chose to
// keep them with the same name. But there are two problems with
// that. First, because I need to wrap them in a std::function
// when promoting to two arguments, I want to make register_style
// themselves take the function parameter by a template argument
// so it doesn't get type-erased "twice" (with two virtual
// calls). But then that means that both versions would have the
// generic signature "template <typename Predicate>
// ... (Predicate, style)" and that would lead to ambiguous calls.
//
// The second problem is that ever if you keep the first parameter
// a std::function, because 'json_pointer' is implicitly
// convertible to a 'json', if you rely on the implicit conversion
// to std::function then you'd get an ambugious call.
//
// So what we want is, using Concept terms:
//
// template <JsonCallable Predicate> ... (Predicate, style)
// template <JsonPointerCallable Predicate> ... (Predicate, style)
//
// where JsonCallable is additionally *not*
// JsonPointerCallable. The following is my attempt to get that.
I then wrote some code that is similar to this:
#include <functional>
struct Main {};
struct Secondary { Secondary(Main); };
// http://en.cppreference.com/w/cpp/types/void_t
template<typename... Ts> struct make_void { typedef void type;};
template<typename... Ts> using void_t = typename make_void<Ts...>::type;
template <typename, typename = void>
struct can_be_called_with_main : std::false_type { };
template <typename T>
struct can_be_called_with_main<
T,
void_t<decltype(std::declval<T>()(std::declval<Main>()))>
>: std::true_type { };
template <typename, typename = void>
struct can_be_called_with_secondary : std::false_type { };
template <typename T>
struct can_be_called_with_secondary<
T,
void_t<decltype(std::declval<T>()(std::declval<Secondary>()))>
>: std::true_type { };
template <typename Functor>
auto
func(Functor f)
-> typename std::enable_if<can_be_called_with_main<Functor>::value, int>::type
{
return 0;
}
template <typename Functor>
auto
func(Functor f)
-> typename std::enable_if<
can_be_called_with_secondary<Functor>::value
&& !can_be_called_with_main<Functor>::value
, int>::type
{
return 0;
}
auto x1 = func([] (Main) {});
auto x2 = func([] (Secondary) {});
where Main is like 'json' and Secondary like 'json_pointer'.
Problem is it doesn't work -- in the SFIANE context, it looks like
predicates of both `bool (json)` and `bool (json_pointer)` are callable
with both json and json_pointer objects.
In the case of `bool (json)` being called with a 'json_pointer', that
is via the implicit conversion discussed in the comment above. In the
caes of `bool (json_pointer)` being called with a `json`, my guess
as to what is going on is that `json` provides an implicit to-anything
conversion, which uses a `from_json` function. However, that isn't
implemented in a SFIANE-friendly way -- when you try to actually make
that conversion, there's a static_assert failure.
An alternative approach would be to extract the first argument to
the provided predicate via some technique like those described in
https://functionalcpp.wordpress.com/2013/08/05/function-traits/,
and then is_same them vs json and json_pointer.
The takes_argument trait is coutesy of @N00byEdge
Hopefully this is an improvement. :-)
|
Hey @EvanED, thanks for opening the PR and the discussion! I am on holidays right now, and I won't be able to join the discussion in the next 3 weeks. Until then, all I can ask you is to provide examples for the new API. While I never doubted that a fully-configurable pretty-printer was possible, I am curious in your proposal how user's of the library can actually use it. Furthermore, backward-compatibility is paramount - I do not want to force users to update, just because the serialization got an API-breaking update. That said, I did not have time to look at your code (and possibly won't for the next weeks). If you already addressed the points above, feel free to ignore this comment :-) |
nlohmann
added
the
state: please discuss
No worries. Enjoy your break! Do you want me to leave this open in the interim?
If you'd like to look at examples currently, I'd recommend looking at the documentation and tests.
I don't think there are any backwards compatibility concerns. My thing is a new API. Now that you mention that though, I could probably implement the current pretty printer in terms of mine, and cut out some code. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
state: stale
|
@EvanED Are you still working on this PR? |
stale
bot
removed
the
state: stale
|
@nlohmann I was waiting on design feedback. :-) (Actually I had kind of forgotten about it for a while, but would have been waiting if I had remembered.) If you like the idea and approach, I can clean up the coveralls problem (codacy I'm not so sure about... at first glance I don't think I should have affected that code, but I can look more) and re-issue the PR as being ready to merge. |
|
Alright, I'll give it a look. Unfortunately, I won't be able to do this in the next 10 days. |
|
In the meantime, could you rebase to the current develop branch? |
|
@EvanED I now had time to look at the PR. I did not find the time to actually play with it, but reading the documentation and looking at the diffs gave me the idea. Wow, this is an amazing extension! I don't think we can get this into the next release, because the SAX interface is already waiting so long that I do not want to postpone it further. For this feature, I would (at least) like the following:
Then I, would like to have another look at the open questions. I think we should aim for a nice sweet spot between possible and actually useful features. For instance, I am not sure whether |
| @@ -7667,7 +7671,7 @@ struct hash<nlohmann::json> | |||
| /// @note: do not remove the space after '<', | |||
| /// see https://github.com/nlohmann/json/pull/679 | |||
| template<> | |||
| struct less< ::nlohmann::detail::value_t> | |||
| struct less<::nlohmann::detail::value_t> | |||
There was a problem hiding this comment.
we should put a // clang-format off comment here
There was a problem hiding this comment.
We are using astyle - it should have a similar option.
There was a problem hiding this comment.
Astyle is already behaving correctly (at least I never had that change when running make pretty), so I believe @EvanED used clang-format.
There was a problem hiding this comment.
I am using the astyle target in the makefile.
One thing to note is that when I got a fresh checkout at the point I started, running that format produced changes -- so I wonder if I'm using a different version or something like that? I think I just apt-got the version with Ubuntu 16.04, but I'm not 100% sure.
There was a problem hiding this comment.
FWIW:
~$ astyle --version
Artistic Style Version 2.05.1
|
@nlohmann First, thanks for the encouragement! I'll take care of the rebase, the other things you mention, and a couple things in my to-do list. I'll close this PR for now I think, and re-issue one in a couple weeks. (Assuming that's the norm? I'm actually not very familiar with the typical GH workflow.) |
|
Thanks a lot! I am looking forward to this! |
|
@EvanED Did you find the time for a new PR? |
|
@EvanED It would be great if you could open a new PR! |
|
Without delving too deep and looking at the code in the PR, is this proposing to add some sort of policy abstraction for the pretty printing? If so, I would very much like to have this functionality. Our project needs pretty printing, however the shape of our json is such that with a few tweaks to the pretty printing code we can dramatically improve the density and visual compactness of our json. At present there doesn't appear to be a way to do this and writing a custom serializer would be overkill. Being able to implement a policy object for the pretty printer could solve this. |
|
Sorry I've been absent for a while; I had a bunch of stuff thrown at me at work and life over the last couple months. :-) I'm still excited to get something like this in, and should have more time in a couple weeks. (@nlohmann) (Sorry, that said months before... I meant weeks.)
Yep, that's exactly what it does. I view it as sort of a CSS analogue. (@abrownsword) |
|
Hey @EvanED, great to know you're still there :-) We had some changes to the serialized lately to configure the reaction to invalid Unicode, see #1314. It again showed how crowded the |
|
Just thought I'd ping this topic again to see if anyone is still thinking about it? |
This is a pretty massive pull request, and is not really ready to be merged yet; there are still a couple things on my to do list. However, I was hoping to open a discussion about the design of this and what you think before completely finishing it out. I'm hoping it's like 70% there. Inspired by #229, #457, maybe others, and of course my own needs/wants. (Almost everything in here is something that is useful to me.) I'm not sure how you want to work; I'm open to instructions otherwise, and I also fully expect to iterate on this at least a couple times.
Let me emphasize that I am primarily focused on feedback on the design, with an eye toward whether you want this in your library. I think the tests and documentation are likely to be the most useful for that, but I don't know how you work. If everything looks great then feel free to take a look at the implementation, but at least for me, that wouldn't be my first stop.
There are a few conceptual groups of changes. I can break these into separate pull requests if you like. I don't know how related or unrelated they should be before they become a "separate issue." Here are changes I made to the stock code, not counting my additions:
make prettyon a cleandevelopcheckout, I got changes. (Artistic Style Version 2.05.1) So there's one commit cleaning that up. Several files have those changes.json_pointer-- though I'm not thrilled with it. See my documentation document for more thoughts on that. (Those are thejson_pointer.hppandunit-json-pointer.cpp)serializerto a newprimitve_serializer. This change also spawned theunit-convenience.cppandunit-coversion.cppchanges. (It became a little more obnoxious to test internals, but I'm thinking this doesn't matter.)Then there are my additions:
fancy_serializer.hppholds the actual codeunit-fancy-serialization.cpphas the tests (I guess now that I'm writing this I'm not sure if I have 100% coverage, but I'd think I'm at least close if not there -- this is a design RFC anyway)json.hpphas a couple trivial changes to make my thing a friendStyledPrettyPrinting.mdhas documentation of the new capabilities. ❗ indicates a couple things I have TODO, and ❓ calls out items I specifically think feedback would be valuable on. If you're a top-down person, this is probably where to start for my additions.Pull request checklist
Read the Contribution Guidelines for detailed information.
include/nlohmanndirectory, runmake amalgamateto create the single-header filesingle_include/nlohmann/json.hpp. The whole process is described here.Please don't
#ifdefs or other means.