Allow configuring spaces before and/or after the colon in DefaultPrettyPrinter

[digulla]

No description provided.

[digulla]

I tried to keep the behavior the same but looking at the broken unit tests, this approach doesn't work very well.

Suggestions how to proceed? Change the tests and risk breaking existing code or create a new, parallel API and deprecate the old ones?

[cowtowncoder]

Yeah this is tricky; whether it's possible to change, in a backwards-compatible way, behavior of DefaultPrettyPrinter.
Especially when Separators is sort-of, kind-of part of public API.

I would probably implement alternate PrettyPrinter implementation, not trying to change existing implementations.

Alternatively if you do want to try to change DefaultPrettyPrinter, I'd be deprecated older methods, add new ones.
I do feel that ability to change spacing around : is useful since ": " seems to be commonly preferred over " : ".

[digulla]

My second idea was to add a boolean spaceBeforeColon which DefaultPrettyPrinter would then read. But some of the spaces handling is in DefaultPrettyPrinter in _withSpaces() and the flag _spacesInObjectEntries.

I think a cleaner approach would be when Separators would contain all the config but I'm not sure whether I can pull that off since the duties are so mixed. I'll try again in the next days.

[cowtowncoder]

@digulla Yeah, apologies for the mess here: division of responsibilities is not well defined for sure. I hope we can figure this out. Ability to include space before and/or after colon sounds like reasonable if it's possible to retro-fit somehow. I assume at least 3 out of 4 combinations would be widely enough used.

[cowtowncoder]

@digulla Ok, looking at Separators / pretty-printers, I agree that Separators probably should have new method(s) for returning "full" separators, in which possible surrounding spaces (if any) are included. Ideally all 3 existing methods would have counterparts, but for now I assume we just need one.
Existing methods would remain, but I think one(s) with overrides should be deprecated, likely.

Now, whether "full" value should be indicated by String containing space, enumeration (4 combinations for pre-/after of single space), or 2 boolean flags per settings I don't know. It might be simplest to just add full String to keep number of settings limited.
Note that default Separators used by MinimalPrettyPrinter would differ from those of DefaultPrettyPrinter: former is compact (no spaces), latter has spaces.

Maybe I could try outlining how I see this working unless you get there first.

EDIT: although, TBH, adding configuration in DefaultPrettyPrinter, as part of calculation of _objectFieldValueSeparatorWithSpaces does look deceptively simple as well. So perhaps that's not a bad alternative; adding an enum outlining combinations of space before/after Object value separator (key/value separator).

[digulla]

I also think we could have a boolean flag which controls the space before the colon but it would make the code even more confusing than it already is.

How about a new implementation for both and deprecating the old ones for 3.x?

I would use PrettyPrinterConfig for the separator strings and StandardPrettyPrinter for the implementation? The config should contain all spaces and the indenters plus a builder to set it up with two defaults (minimal and the current pretty printer).

[digulla]

I've rolled back the incompatible changes and introduced a new Spacing enum. This allows for much cleaner code but one of MinimalPrettyPrinter or DefaultPrettyPrinter can't use Separators.createDefaultInstance() anymore.

Right now, I think is the way to go. DefaultPrettyPrinter should cache the separators instead of calling apply() all the time and all the spacing options in DefaultPrettyPrinter should be deprecated.

[cowtowncoder]

For performance reasons, let's also add the other 2 "WithSpaces" fields that are missing to avoid calling apply() on those cases.

[digulla]

Done

[cowtowncoder]

@digulla I agree this is the way to go. We do need to eliminate calls to apply(), f.ex by adding 2 more fields in DefaultPrettyPrinter. And yes, should deprecate old configuration.

I am hoping to get this merged soon, thank you for working through all the ideas here.

[digulla]

Okay, I feel we're making progress. Have a look at the latest version. It should address all your concerns.

I'm not sure what to do with SerializedString used only for rootSeparator. String is already perfectly serializable; what is the purpose of this class? Should I always use SerializedString? But then, why do the existing fields in DefaultPrettyPrinter mix String and SerializedString?

[cowtowncoder]

Looks good -- I hope I can figure out how to merge it in master/3.0.

[cowtowncoder]

Ok, merged up to master (phew!). Master has a few renamings which complicated things but it should be all good now.

[digulla]

Hey, thanks. It was nice working with you on this.

[cowtowncoder]

@digulla My pleasure, thank you for going through twists & turns :)