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

Drops Java <16 constraint from pretty_format_kotlin #123

Merged

Conversation

harti2006
Copy link
Contributor

Fixes #122

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #123 (52563e6) into master (d9072eb) will not change coverage.
The diff coverage is n/a.

❗ Current head 52563e6 differs from pull request most recent head cf95f11. Consider uploading reports for the commit cf95f11 to get more accurate results

@@            Coverage Diff            @@
##            master      #123   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          305       303    -2     
=========================================
- Hits           305       303    -2     
Impacted Files Coverage Δ
...ormatters_pre_commit_hooks/pretty_format_kotlin.py 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@macisamuele macisamuele left a comment

Choose a reason for hiding this comment

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

Please remove test annotations such that we will know if all test passes.
Once test are green I'll be happy to merge and release this.

@harti2006 thanks for preparing the PR

Please rebase the changes on top of 07944ad as it fixes some possible pre-commit failures.

@harti2006 harti2006 force-pushed the feature/drop-ktlint-java-limitation branch 2 times, most recently from b5b3b38 to 52563e6 Compare December 5, 2022 06:22
@@ -17,7 +17,7 @@ jobs:
os: [macos-latest, ubuntu-latest, windows-latest]

golang_version: [1.19.3]
java_version: ['15', '16', '17']
java_version: ['15', '16', '17', '18', '19']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I removed the test annotations, I'd also like to show that the tool works with even more recent Java versions than 17

@whiskeysierra
Copy link

whiskeysierra commented Dec 5, 2022

Test fails with:

Caused by: java.lang.reflect.InaccessibleObjectException:
Unable to make field private transient java.lang.Object
java.lang.Throwable.backtrace accessible:
module java.base does not "opens java.lang" to unnamed module @28c71909

Sounds related to:

@whiskeysierra
Copy link

It's probably safe to add --add-opens ... to the java process arguments.

auto-merge was automatically disabled December 5, 2022 12:27

Head branch was pushed to by a user without write access

@harti2006 harti2006 force-pushed the feature/drop-ktlint-java-limitation branch from 52563e6 to 2afa43c Compare December 5, 2022 12:27
@harti2006
Copy link
Contributor Author

Tried it like this:

opens_java_lang = ["--add-opens", "java.base/java.lang=ALL-UNNAMED"]
# ktlint does not return exit-code!=0 if we're formatting them.
# To workaround this limitation we do run ktlint in check mode only,
# which provides the expected exit status and we run it again in format
# mode if autofix flag is enabled
check_status, check_output = run_command(
"java", "-jar", ktlint_jar, "--verbose", "--relative", *opens_java_lang, "--", *_fix_paths(args.filenames)
)

auto-merge was automatically disabled December 5, 2022 14:28

Head branch was pushed to by a user without write access

@harti2006 harti2006 force-pushed the feature/drop-ktlint-java-limitation branch from 2afa43c to cf95f11 Compare December 5, 2022 14:28
@harti2006
Copy link
Contributor Author

The first attempt did not work. The args were parsed by the Ktlint application, rather than the "java" command itself. Trying to move them earlier

@macisamuele
Copy link
Owner

Thanks for the effort on making new JDK available to the tool.

If tests are green this will be merged and within EOD I will release the new version of the pre-commit hooks.

@macisamuele macisamuele merged commit 7e7c0fa into macisamuele:master Dec 5, 2022
@harti2006 harti2006 deleted the feature/drop-ktlint-java-limitation branch December 5, 2022 15:12
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.

Unblock ktlint with JDK 17
3 participants