Formatting based on external formatter #80

cottand · 2024-06-01T23:09:44Z

In the spirit of #68 (comment), this PR adds the capability to format Nix files via an external program.

Tested only with nixpkgs-fmt.

Closes #23

This also adds a settings page for Nix under the Languages section, because I felt 'build tools' was not an appropriate section for code formatting.

I have not written Java in a while, so do feel free to call me out on code quality or structure!

JojOatXGME

Many thanks for the PR. Looks good on a high level. I was somehow still accumulating some comments during my review, but there should be nothing big. Would also be nice if you could add something to the CHANGELOG.md.

PS: I also want to note that the Language Server (LSP) may support formatting. It could make sense to disable the formatter of the language server if another formatter is configured. However, I think it is not that important right now.

src/main/java/org/nixos/idea/format/NixExternalFormatter.java

JojOatXGME · 2024-06-02T20:48:44Z

src/main/java/org/nixos/idea/format/NixExternalFormatter.java

+                            if (exitCode == 0) {
+                                request.onTextReady(getOutput().getStdout());
+                            } else {
+                                request.onError("NixIDEA", getOutput().getStderr());


question (non-blocking): Should we maybe use “Nix External Formatter” for the title? (Same in catch block.)

I think it is clearer to use the plugin name as title, so it is clear who is 'to blame' for the error, but I do not have a strong opinion.

I can change it to Nix External Formatter if you prefer

I am fine with both. I think both have their pros and cons. 😅

src/main/java/org/nixos/idea/lsp/ui/CommandSuggestionsPopup.java

JojOatXGME · 2024-06-02T22:12:54Z

src/main/java/org/nixos/idea/settings/NixLangSettings.java

+import java.util.Deque;
+
+@State(name = "NixLangSettings", storages = @Storage(value = "nix-idea-tools.xml", roamingType = RoamingType.DISABLED))
+public final class NixLangSettings implements PersistentStateComponent<NixLangSettings.State> {


polish: I would prefer if you keep this state class more specific for now. Like NixExternalFormatterSettings. Here are my two reasons:

I don't want to rule out that we may add a simple built-in formatter to the plugin at some point.

nix-idea-tools.xml is intended to contain configurations related to external dependencies, like installed applications. There may be other configurations in the future, which should not go into this file.

for reason (1), if you add an non-external formatter, I think it would appropriate to add its settings to this class also (rather than the existing settings in the "Build" section)

for reason (2), if you prefer a different file to nix-idea-tools.xml, we can change that. I am not very familiar with how the @State and @Storage abstraction works, I just wanted a settings screen for this plugin under the "Languages" section.

New settings can go underneath like in the picture below:

I renamed 'Formatter Configuration' to ' External Formatter Configuration'

If you still feel like the external formatter deserves its own section, I am happy to make another one too though! Again, no strong opinions from me on this

My concern was mostly about where and how to store the configuration, not the UI. The configuration of a built-in formatter would be system-independent and can be synchronized across operating systems. External formatters should not be synchronized across operating systems. To make this separation, these two cases need to be stored in separate XML files.

The nix-idea-tools.xml is supposed to become the file storing these system-dependent configurations. This means your initial choice of that XML file was actually correct. Since this XML file will not contain other language-related configurations, I just don't want to call it NixLangSettings.

<application> <component name="NixLspSettings">  </component> <component name="NixExternalFormatterSettings">  </component> </application>

instead of

<application> <component name="NixLspSettings">  </component> <component name="NixLangSettings">  </component> </application>

I see, thanks for the explanation! That makes sense, I will apply your suggestion then.

src/main/java/org/nixos/idea/settings/NixLangSettingsConfigurable.java

JojOatXGME · 2024-06-02T22:23:32Z

src/main/java/org/nixos/idea/settings/NixLangSettingsConfigurable.java

+import javax.swing.*;
+import java.util.List;
+
+public class NixLangSettingsConfigurable implements SearchableConfigurable, Configurable.Beta {


note: You could also use Kotlin for this class if you like. It may help because it allows you to use Kotlin UI DSL 2. I started to use it for NixSymbolConfigurable.kt and NixSymbolSettings.kt at #79 (not yet merged), just in case you want to take a lock. However, I have not yet looked into how to combine that with the CommandSuggestionsPopup.

On a related note, if you are hoping to migrate the plugin to Kotlin entirely, I am happy to help in separate PRs!

Not really, At least not yet. I think the language has its pros and cons. While I will not reject any new feature just because it is implemented in Kotlin, I am still a bit on defense about the language. While it has many nice features, I also feel it tends to encourage some bad patterns. At least what I consider bad patterns. Examples would be excessive use of extension functions and top-level methods. So, I think I will probably experiment with Kotlin a bit at selected places, before I would replace/rewrite existing logic.

fair enough! I am partial to Kotlin, especially for more concurrency-heavy code, but it does take discipline. Having worked with both though, my experience is it can be error-prone to mix with Java.

They have very different code structure philosphies, and the type system can trip you up nullability and concurrency wise, so beware with keeping both around. It might be more sensible to stick to Java until you feel comfortable making the jump (if at all).

edit:

bad patterns (...) use of top-level methods

My personal opinion is that this divide comes from the functional-programming world. Kotlin is not a 'nicer Java'. After a while with Kotlin you will find yourself writing more small data classes and top-level functions - instead of static members and abstract classes

on that spirit, I prefer keeping this code in Java for consistency. I also feel like JB's Kolin UI DSL is not mature enough and requires using Java classes anyway (for the Command box for example)

my experience is it can be error-prone to mix with Java.

I am only aware of potential issues when nullability when you call Java-code from Kotlin. Especially when using Generics, as you can see at gradle/gradle#12388. I haven't noticed any other problems yet with my rather limited experience. At least I didn't have any problems so far when calling Kotlin code of IDEA from Java. Of course, the coroutines and some other constructs don't exist in Java and cannot be used when Java is involved. Do you have any specific hints about potential problems?

you will find yourself writing more small data classes and top-level functions - instead of static members and abstract classes

I think I use a lot of small data classes in Java already and try to avoid abstract classes. Regarding top-level functions: I think it is really nice in Java that you have a namespace matching the file name attached to all methods and other constructs. It allows you to identify the file where the construct is defined. (At least as long as you are following the practice of naming the file according to the class, which is very common practice and partially enforced by the compiler.) I regularly struggle to find the correct file when looking at Kotlin code at GitHub. I don't think that I will ever be converted to top-level functions, assuming I haven't missed some obvious solution to look up the file which defines a method without searching through other files. 😄 Before I worked with Java, I used C++, which has the same problem that you cannot identify the source of a symbol without tooling, which I also always fond quite annoying.

Do you have any specific hints about potential problems?

the painful bits imo come from using non-Kotlin frameworks

serialisation libs don't know about Kotlin nullability and sealed classes

Java collections are seen as 'read-only' from Kotlin (but Java libs can still mutate them)

like you said, anything backend-related will not let you use coroutines. Chances are you will start putting Java-blocking operations into your coroutine dispatchers and unepxectedly blocking all threads in a pool

when working with legacy, a lot of Java code is not annotated, making nullability a guessing game

for this plugin specifically, expect IDEA APIs (which are originally Java) to not be complete in pure Kotlin. You then have to go investigating how to revert back to using old Java classes (usually imperative) within modern Kotlin API (usually declarative).

Ultimately the issues you bump into will probably depend on the stack you work on! I remember suffering with JOOQ too, before they had a Kotlin API.

I regularly struggle to find the correct file when looking at Kotlin code

That's fair. I have gotten used to using Ctrl+F and /-shortuct in github to find symbols. I now work in Go which also encourages top-level functions a lot, and Nix also has this tendency.

If you are curious, I encourage you to try out a pure-Kotlin project to see how it goes :)

cottand · 2024-06-03T07:39:18Z

Thanks for your review! I will address over the next couple days.

PS: I also want to note that the Language Server (LSP) may support formatting. It could make sense to disable the formatter of the language server if another formatter is configured. However, I think it is not that important right now.

I have been using the LSP with this plugin since it released on the plugin marketplace, and I did not manage to make the formatting work via the IntelliJ shortcut.

JojOatXGME · 2024-06-03T18:59:15Z

I have been using the LSP with this plugin since it released on the plugin marketplace, and I did not manage to make the formatting work via the IntelliJ shortcut.

According to the documentation, formatting over LSP should be supported since 2023.3. Note that nil does not support formatting out of the box. You have to manually configure an external formatter.

External formatter must be manually configured to work. See docs/configuration.md for more information.
— https://github.com/oxalica/nil/blob/ab3ddb8f063774cf7e22eb610f5ecfdb77309f3c/docs/features.md

Anyway, I haven't tried it. So maybe it is just not working for some reason. 🤷‍♂️

cottand · 2024-06-03T19:42:10Z

Good catch on the 2023.3 changelog. According to the docs you linked for nil:

The place to write LSP configurations differs between clients (i.e. editors and editor plugins). Please refer to their corresponding documentation

I don't see an API on IDEA's side where I can set this though 🤔

JojOatXGME · 2024-06-03T21:13:32Z

src/main/java/org/nixos/idea/settings/NixLangSettings.java

+import java.util.Deque;
+
+@State(name = "NixLangSettings", storages = @Storage(value = "nix-idea-tools.xml", roamingType = RoamingType.DISABLED))
+public final class NixLangSettings implements PersistentStateComponent<NixLangSettings.State> {


My concern was mostly about where and how to store the configuration, not the UI. The configuration of a built-in formatter would be system-independent and can be synchronized across operating systems. External formatters should not be synchronized across operating systems. To make this separation, these two cases need to be stored in separate XML files.

The nix-idea-tools.xml is supposed to become the file storing these system-dependent configurations. This means your initial choice of that XML file was actually correct. Since this XML file will not contain other language-related configurations, I just don't want to call it NixLangSettings.

<application> <component name="NixLspSettings">  </component> <component name="NixExternalFormatterSettings">  </component> </application>

instead of

<application> <component name="NixLspSettings">  </component> <component name="NixLangSettings">  </component> </application>

JojOatXGME · 2024-06-03T21:19:40Z

.gitignore

@@ -23,3 +23,5 @@ hs_err_pid*
 *.iml
 **/.gradle
 build
+
+src/gen


note: This directory is no-longer generated, which is why I removed it from the .gitignore. However, it probably makes sense to keep the ignore-rule for some more time, as it might be confusing when you temporarily checkout an older commit and then have all these untracked files afterward.

ah that explains why I had these files even though they were not gitignored

src/main/resources/META-INF/plugin.xml

…tings

cottand · 2024-06-07T09:38:36Z

@JojOatXGME sorry to chase, I just wanted to drop a friendly reminder that this PR is ready for review once again. Thank you! 🙏

- Use consistent option names in settings - Introduce StoragePaths - Remove comma in changelog entry - Remove unused NOTIFICATION_GROUP_KEY - Fix some formatting

JojOatXGME

Thanks for the reminder @cottand. I was aware of the PR, but unfortunately I didn't find the time/energy to look into this PR after work during the last days. I made a few adjustments at bf1c923, I hope nothing controversial 😅. If you want to squash the commits or so, feel free to do so now. You can also leave it as is, if you like. I would like to merge the PR during the weekend.

cottand · 2024-06-07T23:34:00Z

No worries at all! Thanks for accepting the PR, and sorry for hurrying you up. bf1c923 is uncontroversial indeed 👌

Feel free to merge whenever. My personal preference is a "squash and merge" if you like, but I will leave it to you

cottand added 3 commits June 2, 2024 00:01

Formatting based on external formatter

e2d42bc

satisfy linter

794f80a

remove unused import

b25a9eb

cottand force-pushed the nixpkgs-formatter branch from 32caa97 to b25a9eb Compare June 2, 2024 06:59

JojOatXGME requested changes Jun 2, 2024

View reviewed changes

cottand added 3 commits June 3, 2024 21:12

address review: feed file contents async

8074155

remove src/gen

791f5b4

address review

8d08642

JojOatXGME reviewed Jun 3, 2024

View reviewed changes

cottand added 3 commits June 3, 2024 22:42

address review

0a67c4f

rename stateful OS-dependent lang settings to NixExternalFormatterSet…

cb81940

…tings

remove unused bundle id in plugin.xml

b3c2898

cottand requested a review from JojOatXGME June 3, 2024 22:02

add formatting to changelog

68d26f1

Some adjustments after review

bf1c923

- Use consistent option names in settings - Introduce StoragePaths - Remove comma in changelog entry - Remove unused NOTIFICATION_GROUP_KEY - Fix some formatting

JojOatXGME approved these changes Jun 7, 2024

View reviewed changes

JojOatXGME merged commit 0a5b4e4 into NixOS:master Jun 8, 2024
5 checks passed

cottand deleted the nixpkgs-formatter branch June 8, 2024 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formatting based on external formatter #80

Formatting based on external formatter #80

cottand commented Jun 1, 2024 •

edited

Loading

JojOatXGME left a comment

JojOatXGME Jun 2, 2024

cottand Jun 3, 2024

JojOatXGME Jun 3, 2024

JojOatXGME Jun 2, 2024

cottand Jun 3, 2024 •

edited

Loading

JojOatXGME Jun 3, 2024

cottand Jun 3, 2024

JojOatXGME Jun 2, 2024 •

edited

Loading

cottand Jun 3, 2024

JojOatXGME Jun 3, 2024 •

edited

Loading

cottand Jun 3, 2024 •

edited

Loading

cottand Jun 3, 2024

JojOatXGME Jun 7, 2024 •

edited

Loading

cottand Jun 7, 2024

cottand commented Jun 3, 2024

JojOatXGME commented Jun 3, 2024

cottand commented Jun 3, 2024

JojOatXGME Jun 3, 2024

JojOatXGME Jun 3, 2024

cottand Jun 3, 2024 •

edited

Loading

cottand commented Jun 7, 2024

JojOatXGME left a comment •

edited

Loading

cottand commented Jun 7, 2024

Formatting based on external formatter #80

Formatting based on external formatter #80

Conversation

cottand commented Jun 1, 2024 • edited Loading

JojOatXGME left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottand Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JojOatXGME Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JojOatXGME Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

cottand Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JojOatXGME Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottand commented Jun 3, 2024

JojOatXGME commented Jun 3, 2024

cottand commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottand Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

cottand commented Jun 7, 2024

JojOatXGME left a comment • edited Loading

Choose a reason for hiding this comment

cottand commented Jun 7, 2024

cottand commented Jun 1, 2024 •

edited

Loading

cottand Jun 3, 2024 •

edited

Loading

JojOatXGME Jun 2, 2024 •

edited

Loading

JojOatXGME Jun 3, 2024 •

edited

Loading

cottand Jun 3, 2024 •

edited

Loading

JojOatXGME Jun 7, 2024 •

edited

Loading

cottand Jun 3, 2024 •

edited

Loading

JojOatXGME left a comment •

edited

Loading