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

rectify palantir configure issue #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

NathanQingyangXu
Copy link
Contributor

No description provided.

Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

I'm a bit worried about removing .editorconfig entirely. There is always going to be a default one, and starting with Google one and modifying it ourselves may be better than none at all.

What do you think?

@@ -62,7 +62,7 @@ spotless {

removeUnusedImports()

palantirJavaFormat(libs.versions.palantir.get()).style("GOOGLE").formatJavadoc(true)
palantirJavaFormat(libs.versions.palantir.get()).formatJavadoc(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is libs.versions.palantir.get() the default? If so, do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked. The default version is based on the following internal "matrix" recommendation:

private static final Jvm.Support<String> JVM_SUPPORT = Jvm.<String> support(NAME).add(8, "1.1.0").add(11, "2.28.0").add(21, "2.39.0");

So if our Java version is 17, 2.28.0 will be chosen.

I missed such subtlety previously and use the latest version 2.50.0 at libs.versions.palantir. Now I am not sure it is a good idea and it seems the default recommended version (i.e. 2.28.0) is enough?

Do you have more experience on palantir? It is totally new to me. As a starter, I think maybe relying on its internal recommendation based on java version is enough (we could specify version later if we found some defect which was fixed in later version?)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experimented locally and found if we use default version (2.28.0 corresonding to java 11), the spotlessJavaDiagnose task will complain as below:

Cannot enable formatJavadoc option, make sure you are using Palantir with version 2.36.0 or later

So maybe using the latest version is a good choice?

@NathanQingyangXu
Copy link
Contributor Author

NathanQingyangXu commented Oct 22, 2024

I'm a bit worried about removing .editorconfig entirely. There is always going to be a default one, and starting with Google one and modifying it ourselves may be better than none at all.

What do you think?

I hesitated a lot as well. Yeah, .editorconfig is a practical way to ensure developer won't be troubled with common format issue (Palantir IDE plugin seems not even working from my manual testing), so it seems a basic one goes a long way.

It is difficult to get one .editorconfig for palantir, but it is easy to get one from google-java-format (in Intellij IDE, after we install google-java-format, we could export a corresponding .editorconfig file from there). Currently the main difference is we need to change its indent width from 2 to 4.

Without .editorconfig and IDE plugin, for palantir the only option is to run gradle task (e.g. spotlessJavaApply). That seems far from being ideal.

could we revert the old .editorconfig (I exported from IDE after google-java-format plugin is installed. But I am not sure whether changing indent width from 2 to 4 is enough.

@NathanQingyangXu NathanQingyangXu changed the title rectify palantir configure issue; delete .editorconfig rectify palantir configure issue Oct 22, 2024
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.

2 participants