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

Implement JDT compiler settings writing #407

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Conversation

spangaer
Copy link
Contributor

Hi,

I'm so happy to see a reboot of this plugin.
I've been using it for driving ScalaIDE based development for over half a decade.
Since ScalaIDE is dead in the water it has also served to make the Java part of a project work in Metals/VSCode
scalameta/metals-feature-requests#5

There it suffered from one issue.
image

Which could be solved by inserting following file: .settings/org.eclipse.jdt.core.prefs

eclipse.preferences.version=1
org.eclipse.jdt.core.compiler.codegen.targetPlatform=1.8
org.eclipse.jdt.core.compiler.compliance=1.8
org.eclipse.jdt.core.compiler.source=1.8

So far I simply checked this file in. That has work for quite a while, but I recently noticed that if the file exists RedHat Java LSP fills this file up with a ton of extra's, which messes up a diff.
image

Hence I felt that creating an extension on this plugin is the better way forward.

Concretely, what this PR does is

  1. check if the file exist
  2. if the file doesn't exit, write those 3 settings like for the other settings files
  3. if the file did exist updated those 3 settings, but only write if there's updates

In both cases it matches the Java version as specified by the container.
I parse the container string, because in the context I need it it's readily available (without having to pass the environment) and it carries the required information.

I also considered whether some conditionality is needed, but because the information that gets written should always be valid, and it won't touch any other settings in the same prefs file I opted to just always apply it.

For lack of a formatter being defined in the repo I applied a local formatter on the changed parts.

Please let me know if something should be added or done differently?

@mkurz
Copy link
Member

mkurz commented Oct 20, 2022

Thanks, I am not so deep into this eclipse config thing, but your change seems legit to me.
It might be a good idea however to also add a test for this. There are scripted tests already setup up: https://github.com/sbt/sbt-eclipse/tree/main/src/sbt-test/sbteclipse
I think it makes sense to test that if that config file exists that you update it correctly and don't touch any other setting. Also it's good for you to test so you are sure this works for the years to come.
Maybe you can just re-use one to the existing scripted tests, I didn't check.

After that I can immediately release a new version.

@spangaer
Copy link
Contributor Author

I'll be looking whether I can make some test materialize.

Next to that, while using this I ran in to the following:

  1. The VSCode setting java.format.settings.url nowadays requires to be a real URL. so I had to introduce file:
  2. Because Red Hat LSP failed to see the formatting profile, it wrote some “default“ profile in to the jdt core prefs
  3. Once the settings are written in the prefs file, they overrule the formatting profile

Because of that I will add a write mode setting that allows to choose to not write, update or overwrite/clean-write, defaulting to not write.

@spangaer
Copy link
Contributor Author

It took me some time to bring this back, but finally got to that.

Now a new setting, which can be passed both as command arg as with a settings key, and allows to Ignore, Remove, Overwrite or Update. Defaulting to Ignore.

I guess I might not have covered all possible combinations, but I think the test is complete enough as is.

@spangaer
Copy link
Contributor Author

I guess the wiki could also use an update?

@mkurz
Copy link
Member

mkurz commented Nov 18, 2022

I guess the wiki could also use an update?

Please, go for it 😉

@mkurz
Copy link
Member

mkurz commented Nov 18, 2022

So this pr is ready for review?

@spangaer
Copy link
Contributor Author

So this pr is ready for review?

As far as I'm concerned as good as it will get.

I guess the wiki could also use an update?

Please, go for it 😉

That's the really fun part isn't it 😛 .

I'll see if I can write something there.

@spangaer
Copy link
Contributor Author

So sadly I can't fork wiki or make a PR for it, so I'll make an update and push it somwhere and you'll have to pull and push it seems.

@spangaer
Copy link
Contributor Author

So I thought I was finding markdown, turns out it was something completely different which ended with installing a beautiful python soup, to get some local preview remotely working.

Anyhow, please fetch from here:
https://github.com/spangaer/sbt-eclipse.wiki.git
and push it to the wiki repo:
https://github.com/sbt/sbt-eclipse.wiki.git

@mkurz
Copy link
Member

mkurz commented Nov 18, 2022

Thanks, I will have a look later.
So if we merge that I guess you expect a new release? Or is there more coming?

@spangaer
Copy link
Contributor Author

I think this is it for now.

If there's no reason to delay such a release, I believe that's better than leaving things in inventory, but I've published a build of the same internally. If there's good reasons to hold it up that's also fine, we're not blocked until this is available.

Just a matter of sharing a solution for a problem we face.

@mkurz
Copy link
Member

mkurz commented Nov 18, 2022

Release will follow immediately after I have time to review.

@spangaer
Copy link
Contributor Author

Poke

@mkurz
Copy link
Member

mkurz commented Jan 24, 2023

@spangaer Thanks for the ping, will take a look asap.

@spangaer
Copy link
Contributor Author

🔔

@spangaer
Copy link
Contributor Author

spangaer commented Dec 8, 2023

image

@mkurz mkurz merged commit 2873967 into sbt:main Dec 11, 2023
6 checks passed
@mkurz
Copy link
Member

mkurz commented Dec 11, 2023

@spangaer Thanks and sorry for the very long delay. Released v6.1.0 and also pushed your wiki changes:

@spangaer spangaer deleted the oss/jdt-settings branch December 15, 2023 11:00
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