Add support for Palantir Java Formatter #213

eirnym · 2024-02-14T14:28:35Z

Palantir Java Formatter has a bit different settings than Google Java Format.

As the original project doesn't provide standalone version, repackager CI has been used.

language_formatters_pre_commit_hooks/pretty_format_java.py

tests/pretty_format_java_test.py

macisamuele

Change looks good and thanks for updating the automation to automatically update the tool versions.
If tests are green I'll be happy to merge and release this change

eirnym · 2024-03-31T21:02:03Z

Palantir Java formatter currently requires Java Version < 21 and is not available if java version is above 21, so coverage won't show 100% if it's the case.

macisamuele · 2024-03-31T21:09:29Z

If this is the case we need to make sure that

at runtime we check the jdk in use and bail out if not on the proper version (there is already an helper for that)
have unit test verifying that
have tests that run the tool end to end with pytest.skipif tk make sure that the test is only run if new sdk available

Once the points above are verified tests will be green and we could merge this

eirnym · 2024-03-31T23:34:01Z

I think I have this test, I'll double check it.

eirnym · 2024-03-31T23:34:06Z

Anyway this won't help with coverage that you'd like to be 100%

eirnym · 2024-03-31T23:37:27Z

Do you mind if I'll split tests for google formatter and palantir into separate files?

macisamuele · 2024-04-01T02:34:57Z

Do you mind if I'll split tests for google formatter and palantir into separate files?

I would rather prefer to keep the tests of a single module into a single test file, it makes it managing them more predictable.

Test coverage can be kept high, it is sufficient to check what the test is mentioning as not covered lines

  py310: commands[2]> coverage report
  Name                                                         Stmts   Miss  Cover   Missing
  ------------------------------------------------------------------------------------------
  language_formatters_pre_commit_hooks/pretty_format_java.py      54      1    98%   220
  ------------------------------------------------------------------------------------------
  TOTAL                                                          356      1    99%

Checking the code I see that the following line is not covered

language-formatters-pre-commit-hooks/language_formatters_pre_commit_hooks/pretty_format_java.py

Line 220 in 881f4e1

cmd_args.append("--replace")

The coverage missing is due to the fact that we cannot guarantee that end-to-end test is run with Java 21 in all the environments, hence let's add # pragma: nocover for now to silence the coverage report and we can check it on a second round.

PS: I would suggest to rebase the branch on the latest master branch such that you'll have tests run with Java 21 as well (#219)

eirnym · 2024-04-01T09:48:53Z

@macisamuele I've done as you said.

rebased to master
added nocover for replace flag
Fixed one test name
passed JAVA_HOME to tox (as on a local computer many Java version are installed)

For java pre 21, coverage is 100%, while on java 21 palantir coverage won't pass because of test precondition.

eirnym · 2024-04-11T14:04:45Z

@macisamuele any news?

codecov · 2024-04-12T17:53:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2f03d55) to head (b879caa).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #213   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          355       371   +16     
=========================================
+ Hits           355       371   +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

macisamuele

Change looks overall good.

language_formatters_pre_commit_hooks/pretty_format_java.py

setup.cfg

language_formatters_pre_commit_hooks/pretty_format_java.py

.pre-commit-hooks.yaml

tests/pretty_format_java_test.py

eirnym · 2024-04-16T11:02:47Z

@macisamuele you can't guarantee full coverage on Java 21, as tool doesn't support Java 21 at all, as it just don't run because of internal dependencies and slow development process (due to licensing). I can comment all lines for Palantir code as # pragma: nocover if you see this would be a solution

CopperGiraffe · 2024-05-13T13:42:44Z

Hello there, Palantir support would be great, is there any news on the subject?

[Palantir Java Formatter](https://github.com/palantir/palantir-java-format) has a bit different settings than Google Java Format.

* Rename version file from palantir_java_format.version to palantir.version * Update palantir to 2.43.0 * Add test for Java 21

Palantir doesn't run on Java >=21

eirnym · 2024-07-03T14:21:27Z

@macisamuele Palantir fixed the issue and now supports Java 21. I updated version, rebased and added tests. Could you please review it again?

.github/workflows/bump_external_releases.py

mxr · 2024-07-03T18:48:12Z

language_formatters_pre_commit_hooks/pretty_format_java.py

+    def get_urls(_version: str) -> typing.List[str]:
+        # Links extracted from https://github.com/jsonschema2dataclass/palantir-cli
+        return [
+            "https://github.com/jsonschema2dataclass/palantir-cli/releases/download/"
+            "{version}/palantir-cli-{version}-standalone.jar".format(
+                version=_version,
+            ),
+        ]
+
+    possible_urls = get_urls(version)


I know this is copied from other parts of the code but IMO I would inline this function

macisamuele · 2024-07-03T21:12:25Z

Please address coverage and then ready to merge.

language_formatters_pre_commit_hooks/pretty_format_java.py      59      3    95%   233-239

eirnym · 2024-07-03T23:01:02Z

@macisamuele Finally nailed it!

eirnym · 2024-07-12T23:24:33Z

Any news?

macisamuele · 2024-07-13T12:05:47Z

Had a very busy time in the last days.
Enabled tests to run and if they pass this PR will be merged.
Tomorrow I'll check the status of this and release a new version if the change was merged

macisamuele · 2024-07-13T12:41:01Z

Change is merged , at the same time pre-commit were not really fixed.
Going to fix them directly on master and take care of the release

eirnym · 2024-07-13T15:11:43Z

Thank you!

eirnym commented Feb 14, 2024

View reviewed changes

language_formatters_pre_commit_hooks/pretty_format_java.py Show resolved Hide resolved

eirnym commented Feb 14, 2024

View reviewed changes

tests/pretty_format_java_test.py Show resolved Hide resolved

macisamuele approved these changes Mar 31, 2024

View reviewed changes

macisamuele enabled auto-merge March 31, 2024 17:04

auto-merge was automatically disabled March 31, 2024 17:54
Base branch was modified

eirnym force-pushed the palantir branch from 881f4e1 to 2a33013 Compare April 1, 2024 09:44

eirnym requested a review from macisamuele April 4, 2024 20:06

macisamuele approved these changes Apr 12, 2024

View reviewed changes

language_formatters_pre_commit_hooks/pretty_format_java.py Outdated Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

language_formatters_pre_commit_hooks/pretty_format_java.py Outdated Show resolved Hide resolved

mxr reviewed Apr 13, 2024

View reviewed changes

.pre-commit-hooks.yaml Show resolved Hide resolved

mxr reviewed Apr 13, 2024

View reviewed changes

tests/pretty_format_java_test.py Show resolved Hide resolved

eirnym added 5 commits July 3, 2024 16:11

Add support for Palantir Java Formatter

a93186b

[Palantir Java Formatter](https://github.com/palantir/palantir-java-format) has a bit different settings than Google Java Format.

Minor fixes for tests and for output

6db5f24

pass JAVA_HOME to tox tests

9f6054f

Fixes after review

ad857a0

* Rename version file from palantir_java_format.version to palantir.version * Update palantir to 2.43.0 * Add test for Java 21

add pragma no cover to the format_palantir function

4ca55b7

Palantir doesn't run on Java >=21

eirnym force-pushed the palantir branch from b33bcba to 41a40ef Compare July 3, 2024 14:20

eirnym force-pushed the palantir branch from 41a40ef to ce96611 Compare July 3, 2024 14:21

eirnym requested a review from mxr July 3, 2024 14:27

eirnym requested a review from macisamuele July 3, 2024 14:27

eirnym force-pushed the palantir branch 2 times, most recently from 6f10286 to c9adebb Compare July 3, 2024 14:53

mxr reviewed Jul 3, 2024

View reviewed changes

.github/workflows/bump_external_releases.py Outdated Show resolved Hide resolved

eirnym requested a review from mxr July 3, 2024 16:02

eirnym force-pushed the palantir branch from c9adebb to 0378a61 Compare July 3, 2024 16:05

mxr reviewed Jul 3, 2024

View reviewed changes

mxr approved these changes Jul 3, 2024

View reviewed changes

macisamuele approved these changes Jul 3, 2024

View reviewed changes

Palantir support Java 21+ as well

b879caa

eirnym force-pushed the palantir branch from 0378a61 to b879caa Compare July 3, 2024 22:57

macisamuele enabled auto-merge July 13, 2024 12:04

macisamuele merged commit 37ef1db into macisamuele:master Jul 13, 2024
19 of 23 checks passed

macisamuele added a commit that referenced this pull request Jul 13, 2024

Fix pre-commit on altered files in #213

1fcd2d1

macisamuele mentioned this pull request Jul 13, 2024

GitHub actions: fix and bump #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Palantir Java Formatter #213

Add support for Palantir Java Formatter #213

eirnym commented Feb 14, 2024

macisamuele left a comment

eirnym commented Mar 31, 2024

macisamuele commented Mar 31, 2024 •

edited

Loading

eirnym commented Mar 31, 2024

eirnym commented Mar 31, 2024

eirnym commented Mar 31, 2024 •

edited

Loading

macisamuele commented Apr 1, 2024 •

edited

Loading

eirnym commented Apr 1, 2024 •

edited

Loading

eirnym commented Apr 11, 2024

codecov bot commented Apr 12, 2024 •

edited

Loading

macisamuele left a comment

eirnym commented Apr 16, 2024

CopperGiraffe commented May 13, 2024

eirnym commented Jul 3, 2024

mxr Jul 3, 2024

macisamuele commented Jul 3, 2024

eirnym commented Jul 3, 2024

eirnym commented Jul 12, 2024

macisamuele commented Jul 13, 2024

macisamuele commented Jul 13, 2024

eirnym commented Jul 13, 2024

Add support for Palantir Java Formatter #213

Add support for Palantir Java Formatter #213

Conversation

eirnym commented Feb 14, 2024

macisamuele left a comment

Choose a reason for hiding this comment

eirnym commented Mar 31, 2024

macisamuele commented Mar 31, 2024 • edited Loading

eirnym commented Mar 31, 2024

eirnym commented Mar 31, 2024

eirnym commented Mar 31, 2024 • edited Loading

macisamuele commented Apr 1, 2024 • edited Loading

eirnym commented Apr 1, 2024 • edited Loading

eirnym commented Apr 11, 2024

codecov bot commented Apr 12, 2024 • edited Loading

Codecov Report

macisamuele left a comment

Choose a reason for hiding this comment

eirnym commented Apr 16, 2024

CopperGiraffe commented May 13, 2024

eirnym commented Jul 3, 2024

mxr Jul 3, 2024

Choose a reason for hiding this comment

macisamuele commented Jul 3, 2024

eirnym commented Jul 3, 2024

eirnym commented Jul 12, 2024

macisamuele commented Jul 13, 2024

macisamuele commented Jul 13, 2024

eirnym commented Jul 13, 2024

macisamuele commented Mar 31, 2024 •

edited

Loading

eirnym commented Mar 31, 2024 •

edited

Loading

macisamuele commented Apr 1, 2024 •

edited

Loading

eirnym commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 12, 2024 •

edited

Loading