search

jakartaee / jsonp-api

Jakarta JSON Processing
https://eclipse.org/ee4j/jsonp
Other
141 stars 61 forks source link

Issue #190 - avoid writing newline before first object/array #224

Closed lmsurpre closed 4 years ago

lmsurpre commented 4 years ago

  1. Added a protected inNone() method to JsonGeneratorImpl so that subclasses can determine when we are in the outermost scope. I'm not attached to the name.

  2. Added a check to the if statement in JsonPrettyGeneratorImpl.writeComma() to avoid writing the newline and indentation when we're in the root context.

Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com

lmsurpre commented 4 years ago

@nmittles Your proposed fix was my first attempt, but it turns out it fails to print the newline after the initial { in that case.

The issue is that we want JsonPrettyGeneratorImpl.writeComma() to generate a newline (but sometimes with no comma) whenever we're NOT in the outermost scope.

I considered an alternative where I would split the write newline out of writeComma, but that felt like a bigger change than I wanted to propose.

lmsurpre commented 4 years ago

So whether to print the newline is actually independent of whether its the first elements or not.

In fact, the tests still pass when I remove the isCommaAllowed() check from JsonPrettyGeneratorImpl.writeComma ... I just wasn't totally confident in the ramifications of losing the currentContext.scope != Scope.IN_FIELD part of that check.

m0mus commented 4 years ago

@nmittles You didn't finish your review. Are you fine to merge it?

lmsurpre commented 4 years ago

I rebased onto master since there was now a merge conflict. @nmittles and @m0mus can you please re-approve and merge this before it goes stale again?