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

chore: get ./gradlew format to pass for the whole repo #29766

Closed
wants to merge 7 commits into from

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Aug 23, 2023

In principle this is the same thing as #29686 but extended to the whole repo. The purpose of this PR is twofold:

  1. educate myself on the different code formatters that we use or intend to use;
  2. if possible identify a java code style which is both pleasant (see chore: commit the result of ./gradlew format #29686 (comment) for concerns about the existing style) and which isn't too different so as to not pollute the git history with more noise than necessary.

In the process of (1) I found that there's not much work required to get the format gradle task to succeed everywhere. This surfaced a few bugs which are perhaps worth fixing.

W.r.t (2) I found that https://github.com/palantir/palantir-java-format is tolerable if we tell it to conform to the Google style. This shouldn't be too surprising as it was derived from the latter originally. It's basically the same thing but with nicer handling of java-8+ lambdas. It seems to have more sophisticated rules than what's possible with the Eclipse CodeFormatter that we currently use. Styles are subjective and can be endlessly bike-shedded but I found the result to be quite nice.

The first 3 commits actually have interesting changes, the rest are mechanical.

@octavia-squidington-iii octavia-squidington-iii added area/api Related to the api area/connectors Connector related issues area/platform issues related to the platform CDK Connector Development Kit labels Aug 23, 2023
@@ -25,5 +25,4 @@ data:
ab_internal:
sl: 200
ql: 300
supportLevel: community
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This key is duplicated and the other entry's value is certified. Which one is correct? I did this change assuming that certified overrides community.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in the connector report the Confluence source shows up as community.

@aaronsteers
Copy link
Collaborator

aaronsteers commented Aug 23, 2023

@postamar - I love this. Nice work. 👍

Sharing back here feedback that I sent also in DM...

  1. I really like the approach taken by https://github.com/palantir/palantir-java-format.
  2. A quick spot check of file diffs applied shows nice improvements that I would agree with in most/all cases.
  3. Although the 120 character limit is more liberal than I prefer personally, it is a very workable constraint which results in nicely readable code for most scenarios. (Side-by-side diff is still rough, but other review modes are quite workable with 120 limit.)
  4. On the Python side, I'd like to see black+ruff applied globally, but that can be saved for a later PR. (Maybe it is already in scope, but either way I think this global pass is worth considering for merge.)

@postamar postamar marked this pull request as ready for review August 23, 2023 22:59
@postamar postamar requested review from a team as code owners August 23, 2023 22:59
@cgardens
Copy link
Contributor

@postamar for #2 is there are a particular issue you have with the existing formatter? I definitely agree, we need to run it for it be useful. Fwiw, the whole platform code base uses the existing formatter. It is not a hard requirement that we stay consistent across the two code bases, but I think we should default to doing that unless we have a strong reason to go in different directions.

@postamar
Copy link
Contributor Author

Closing this in favour of #29786 which is the same thing, but using the existing style. I'll submit a separate PR with the palantir style and we can continue the discussion there @cgardens.

@postamar postamar closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/connectors Connector related issues area/platform issues related to the platform CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants