FormattingStyle follow-up

[Marcono1234]

Follow-up for #2231

• Extends and adds missing documentation
• Fixes some small typos
• Adds tests for null being used as formatting style

[Marcono1234]

Have changed this to @code because this is within the documentation of FormattingStyle, so linking to itself is probably not necessary.

The (somewhat dated) "How to Write Doc Comments for the Javadoc Tool" guide also says: "Use in-line links economically"

[eamonnmcmanus]

I'd be inclined not to do this. To quote Google's internal Best Practices document on javadoc:

Any specific type mentioned in prose should be hyperlinked every time in that documentation block. Historical advice was to link once and only once, but using {@link} every time is simpler, future-proof, and more tool-friendly. IDEs and code indexers can recognize this as a reference to a specific API and make it navigable. As an example, if that API is renamed later, the tools should know to fix this reference.

(Comment applies throughout.)

[Marcono1234]

The point about validating the referenced name makes sense, but to me that appears to be rather a workaround for missing Javadoc functionality (e.g. @code tag which represents a reference).

Personally I still think in the HTML output the links can be distracting / confusing, giving the impression that they refer to a different, similarly named class.

I have reverted these @link changes now though.

[MaicolAntali]

Looks nice, just a small review

[eamonnmcmanus]

Looking at these changes, it strikes me that it's kind of ugly to use null to mean no formatting. Could we have FormattingStyle.NONE instead? I think having empty strings for newline and indent should mean no formatting? Also, we should probably reject having an empty newline with a non-empty indent.

These changes are outside the scope of this PR but I think we should probably make them before freezing the API at the next release.

[eamonnmcmanus]

I'd be inclined not to do this. To quote Google's internal Best Practices document on javadoc:

Any specific type mentioned in prose should be hyperlinked every time in that documentation block. Historical advice was to link once and only once, but using {@link} every time is simpler, future-proof, and more tool-friendly. IDEs and code indexers can recognize this as a reference to a specific API and make it navigable. As an example, if that API is renamed later, the tools should know to fix this reference.

(Comment applies throughout.)

[Marcono1234]

Could we have FormattingStyle.NONE instead? I think having empty strings for newline and indent should mean no formatting?

Though currently JsonWriter sets this.separator = ": " for any non-null formatting style. Maybe there are use cases where that is wanted for empty newline and indentation, but then a special hardcoded check for FormattingStyle.NONE is needed, regardless of whether it has empty newline and indentation. What do you think?

[eamonnmcmanus]

Though currently JsonWriter sets this.separator = ": " for any non-null formatting style. Maybe there are use cases where that is wanted for empty newline and indentation, but then a special hardcoded check for FormattingStyle.NONE is needed, regardless of whether it has empty newline and indentation. What do you think?

I think that suggests that FormattingStyle should also allow users to specify what space comes after : and ,? FormattingStyle.NONE would have an empty string there, and FormattingStyle.DEFAULT would have a single space.

[Marcono1234]

what space comes after : and ,

Currently after a , there is either nothing (compact) or a newline:

gson/gson/src/main/java/com/google/gson/stream/JsonWriter.java

Lines 727 to 729 in ef35a34

	case NONEMPTY_ARRAY: // another in array
	out.append(',');
	newline();

For space after : I am not sure if we really need to expose that, and if the use cases for customizing this are that common. Maybe for now we could store that internally and use no space for NONE, but any customization of NONE implicitly adds a space. If users really request this to be customized we can consider in the future adding API for that.

Also, what about naming NONE → COMPACT, and DEFAULT → PRETTY; that might make their meaning more clear? At least from the perspective that formatting is always applied, even if that means for COMPACT to add no whitespace.

[eamonnmcmanus]

OK, well what do you think of this:

• For this PR, just address the minor issues with javadoc.
• In a later PR:
  • Rename DEFAULT to PRETTY and introduce COMPACT (née NONE). I agree that these are better names.
  • Introduce FormattingStyle.equals with the obvious behaviour.
  • Forbid a null FormattingStyle.

I agree that customizing what comes after , or : is probably not necessary, at least until people ask for it. If you're using a FormattingStyle that equals(FormattingStyle.COMPACT) then there's no whitespace; otherwise we use newline+indent after , and a single space after :. We could add a public method FormattingStyle.isCompact() that caches the value of equals(COMPACT).

[Marcono1234]

Have reduced this now to Javadoc and minimal code changes.

[eamonnmcmanus]

LGTM