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.

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

Format the code following Google Java Style Guide #2288

Closed

Conversation

MaicolAntali
Copy link
Contributor

Reformatted all source files with google-java-format as mentioned in #2284

@MaicolAntali
Copy link
Contributor Author

MaicolAntali commented Dec 16, 2022

Some tests fails, but the issue it's related with the order of the fields in the classes.

For exemple the test: ExposeFieldsTest.testArrayWithOneNullExposeFieldObjectSerialization fails due: expected:<[{"[a":1,"d":2.0},{"d":2.0},{"a":2,"d":2.0]}]> but was:<[{"[d":2.0,"a":1},{"d":2.0},{"d":2.0,"a":2]}]>

The issue here come from the object field order. The class before formatting following the google style giude:

  private static class ClassWithExposedFields {
    @Expose private final Integer a;
    private final Integer b;
    @Expose(serialize = false) final long c;
    @Expose(deserialize = false) final double d;
    @Expose(serialize = false, deserialize = false) final char e;

...

and after:

private static class ClassWithExposedFields {
    @Expose(serialize = false) final long c;
    @Expose(deserialize = false) final double d;
    @Expose(serialize = false, deserialize = false) final char e;
    @Expose private final Integer a;
    private final Integer b;

...

In this case is it better to keep the class in the "old" way or fix the test?
(It’s the same for the other failed tests)

@eamonnmcmanus
Copy link
Member

As mentioned (somewhat hidden) in the issue, I think we should do this reformatting last, after we've addressed the other things such as migrating tests to JUnit 4. But this is still a useful experiment because we certainly wouldn't expect test failures. Did google-java-format reorder those fields? I don't think the Google style guide requires package fields to come before private ones, so that would be a bug. But also, the order in which fields are returned by reflection is explicitly not ordered so it may be that the failing tests are making invalid assumptions.

@Marcono1234
Copy link
Collaborator

Did google-java-format reorder those fields? I don't think the Google style guide requires package fields to come before private ones, so that would be a bug.

It looks like multiple methods and nested classes were reordered as well. The style guide says:

However, there's no single correct recipe for how to do it; different classes may order their contents in different ways.

Personally I also think that no specific ordering should be enforced; that would be too strict and would be more than just formatting.


But also, the order in which fields are returned by reflection is explicitly not ordered so it may be that the failing tests are making invalid assumptions.

Probably related to #617. It looks like the majority of Gson's reflection based tests depend on a consistent order.

@MaicolAntali MaicolAntali deleted the format-google-java-style-guide branch March 6, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants