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

turn off static imports format for java formatter. #756

Closed
wants to merge 1 commit into from
Closed

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Oct 21, 2023

refs: #723 & #593

Update: When I run the format on Windows, there are 314 files change, so better to leave it in this pr.

@He-Pin He-Pin marked this pull request as ready for review October 21, 2023 11:29
.sbt-java-formatter.conf Outdated Show resolved Hide resolved
@He-Pin He-Pin requested a review from mdedetrich October 21, 2023 11:46
@He-Pin He-Pin requested review from Roiocam and pjfanning and removed request for Roiocam October 22, 2023 05:43
@He-Pin He-Pin changed the title Update sbt-java-formatter to 0.8.0 turn off static imports format Oct 22, 2023
@mdedetrich
Copy link
Contributor

mdedetrich commented Oct 22, 2023

I swear I mentioned this before but I guess I was writing a comment on my mobile and it was lost somewhere but I am not sure what the use of upgrading the sbt-java-formatter is if its changing the meaning of the code by incorrectly formatting imports that is forcing us to add files to the ignore list and/or using format: off/format: on. Its not just about the state of the codebase now, if contributors add/modify .java files in the future than it creates an experience that is less than ideal.

It would be far better to figure out the underlying root cause, i.e. maybe there is an option to keep the import order as it was before?

@He-Pin He-Pin changed the title turn off static imports format turn off static imports format for java formatter. Oct 22, 2023
@He-Pin
Copy link
Member Author

He-Pin commented Oct 22, 2023

@mdedetrich There are no config for this formatter.

I was added these file to ignored-files in .sbt-java-formatter.conf , it works too.

When I ran the javafmtAll there are 314 files affected, but seems less when ran the command from Linux.

@mdedetrich
Copy link
Contributor

mdedetrich commented Oct 22, 2023

@mdedetrich There are no config for this formatter.

Then to be frank I think we should look for another formatter. If a formatter is changing the meaning of a program and at least doesn't allow a configuration to prevent that then this is a terrible design.

@pjfanning thoughts?

@He-Pin
Copy link
Member Author

He-Pin commented Oct 22, 2023

@mdedetrich And seems all those tests are related with *Behavior javadsl.

@He-Pin
Copy link
Member Author

He-Pin commented Oct 22, 2023

@mdedetrich but as your comments in anther pr, it's a JDK 8 bug.

@mdedetrich
Copy link
Contributor

@mdedetrich but as your comments in anther pr, it's a JDK 8 bug.

I don't remember saying this, can you provide a link?

@He-Pin
Copy link
Member Author

He-Pin commented Oct 22, 2023

@mdedetrich from #593

https://stackoverflow.com/questions/50145575/why-does-rearranging-imports-cause-compilation-to-fail might be related, in which case we should change the formatting style or add these files into ignore

@mdedetrich
Copy link
Contributor

Ah right now I remember, still at odds on this because we still support JDK 8 and will likely do so for some time (especially if multi release jar module is made). Lets see what other people say

@He-Pin
Copy link
Member Author

He-Pin commented Oct 23, 2023

@mdedetrich I think this is fine, anther workaround is delete the static imports, by this change we can make some progress anyway

@mdedetrich
Copy link
Contributor

@mdedetrich I think this is fine, anther workaround is delete the static imports, by this change we can make some progress anyway

My current stance is to actually drop it and to add sbt-java-formatter to the ignore list for scala steward until we happen to drop JDK 8.

Its not just about current code with the format: off/format: on workaround, any new code that is written in Java can be then be autoformatted to reveal this JDK 8 bug and there is no reason for us to upgrade anyways since we are stuck with "JDK 8" java style.

@He-Pin
Copy link
Member Author

He-Pin commented Oct 23, 2023

Okay, let's close it, and pin the sbt-java-formatter at 0.7.0.

@He-Pin He-Pin closed this Oct 23, 2023
@He-Pin He-Pin deleted the javafmt branch October 23, 2023 08:04
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.

3 participants