Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use non-null FormattingStyle; configure space after separator #2345

Merged
merged 7 commits into from May 30, 2023
Merged

Use non-null FormattingStyle; configure space after separator #2345

merged 7 commits into from May 30, 2023

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Mar 18, 2023 •
edited

Depends on #2327

Purpose

Follow-up for #2327 containing the API changes, see also #2327 (comment).

Description

Also contains an experimental implementation for #2344 (closes #2344). Please let me know what you think, and whether that should be omitted from this pull request.

Have marked this pull request as draft for now to first gather some feedback. This pull request is also not complete; it is missing unit tests.

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

eamonnmcmanus
eamonnmcmanus reviewed Mar 19, 2023
View reviewed changes
Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I like the look of the API changes.

Just some very minor comments.

gson/src/main/java/com/google/gson/stream/JsonWriter.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/stream/JsonWriter.java Show resolved Hide resolved
gson/src/test/java/com/google/gson/functional/FormattingStyleTest.java Outdated
@@ -35,7 +34,7 @@
public class FormattingStyleTest {

private static final String[] INPUT = {"v1", "v2"};
private static final String EXPECTED = "[<EOL><INDENT>\"v1\",<EOL><INDENT>\"v2\"<EOL>]";
private static final String EXPECTED = "[<EOL><INDENT>\"v1\",<SPACE><EOL><INDENT>\"v2\"<EOL>]";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread this as saying that there would be a space before a newline here, which of course would be surprising. Perhaps replace <SPACE><EOL> with <SPACE_OR_EOL> or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have named them COLON_SPACE and COMMA_SPACE, still slightly misleading but hopefully a bit less than before. Are these fine?

@Marcono1234 Marcono1234 force-pushed the marcono1234/pretty-print-followup-2 branch from 3eb18ff to f2e8218 Compare March 23, 2023 23:13
Marcono1234
Marcono1234 commented Mar 23, 2023
View reviewed changes
gson/src/test/java/com/google/gson/functional/FormattingStyleTest.java
Gson gson = new GsonBuilder().setPrettyPrinting(style).create();
String json = gson.toJson(INPUT);
assertThat(json).isEqualTo(buildExpected(newline, indent));
public void testFormat() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined all these separate test methods into a single method which also tests more combinations.

@Marcono1234 Marcono1234 marked this pull request as ready for review March 23, 2023 23:20
@Marcono1234
Copy link
Collaborator Author

The "Check API compatibility" CI task correctly identified incompatible changes here:

Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.17.2:cmp (default-cli) on project gson:
There is at least one incompatibility:
com.google.gson.FormattingStyle.DEFAULT:FIELD_REMOVED
com.google.gson.GsonBuilder.setPrettyPrinting(com.google.gson.FormattingStyle):METHOD_REMOVED

But that is expected and also acceptable I guess because the FormattingStyle changes have not been released yet.

@Marcono1234
Copy link
Collaborator Author

@eamonnmcmanus, could you please have a look? As mentioned above the failure for "Check API compatibility" is expected, since it compares these changes against master, but this pull request refactors some of the not yet released FormattingStyle API.

@Marcono1234
Copy link
Collaborator Author

@eamonnmcmanus, could you please have a look at this pull request when you have some time? Would be good to get this merged (with any adjustments you request) before the next release, since this makes backward-incompatible changes to the not yet released FormattingStyle API, which would not be (easily) possible after it has been released.

@eamonnmcmanus
Copy link
Member

Sorry for the delay. I'll merge this as soon as the automated checks have completed. (For some reason they are remaining queued for a very long time at the moment.)

@eamonnmcmanus eamonnmcmanus merged commit 481ac9b into google:master May 30, 2023
6 of 7 checks passed
eamonnmcmanus added a commit that referenced this pull request May 31, 2023
@Marcono1234 @eamonnmcmanus
Use non-null FormattingStyle; configure space after separator (#2345
5ba96c3 
)

* Use non-`null` `FormattingStyle`; configure space after separator

* Improve Javadoc and tests

* Rename to plural separator*s*

* Add explicit tests for default formatting styles

---------

Co-authored-by: Éamonn McManus <emcmanus@google.com>
@Marcono1234 Marcono1234 deleted the marcono1234/pretty-print-followup-2 branch May 31, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eamonnmcmanus eamonnmcmanus eamonnmcmanus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support single line pretty-printed JSON output
2 participants
@Marcono1234 @eamonnmcmanus