Skip to content

Formatting based on external formatter #80

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jun 8, 2024
Merged

Formatting based on external formatter #80

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e2d42bc
Formatting based on external formatter
cottand Jun 1, 2024
794f80a
satisfy linter
cottand Jun 1, 2024
b25a9eb
remove unused import
cottand Jun 1, 2024
8074155
address review: feed file contents async
cottand Jun 3, 2024
791f5b4
remove src/gen
cottand Jun 3, 2024
8d08642
address review
cottand Jun 3, 2024
0a67c4f
address review
cottand Jun 3, 2024
cb81940
rename stateful OS-dependent lang settings to NixExternalFormatterSet…
cottand Jun 3, 2024
b3c2898
remove unused bundle id in plugin.xml
cottand Jun 3, 2024
68d26f1
add formatting to changelog
cottand Jun 3, 2024
bf1c923
Some adjustments after review
JojOatXGME Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,5 @@ hs_err_pid*
*.iml
**/.gradle
build

src/gen
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cottand cottand Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

23 changes: 14 additions & 9 deletions src/main/java/org/nixos/idea/format/NixExternalFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.nixos.idea.file.NixFile;
import org.nixos.idea.lang.NixLanguage;
import org.nixos.idea.settings.NixLangSettings;

import java.io.IOException;
Expand All @@ -28,7 +29,7 @@ public final class NixExternalFormatter extends AsyncDocumentFormattingService {

@Override
protected @NotNull String getNotificationGroupId() {
return "NixIDEA";
return NixLanguage.NOTIFICATION_GROUP_ID;
}

@Override
Expand All @@ -50,7 +51,6 @@ public boolean canFormat(@NotNull PsiFile psiFile) {
@Override
protected @Nullable FormattingTask createFormattingTask(@NotNull AsyncFormattingRequest request) {
NixLangSettings nixSettings = NixLangSettings.getInstance();
System.out.println("started fmt task");
if (!nixSettings.isFormatEnabled()) {
return null;
}
Expand All @@ -62,14 +62,11 @@ public boolean canFormat(@NotNull PsiFile psiFile) {
var command = nixSettings.getFormatCommand();
List<String> argv = ParametersListUtil.parse(command, false, true);

try {
var commandLine = new GeneralCommandLine(argv);
var commandLine = new GeneralCommandLine(argv);

OSProcessHandler handler = new OSProcessHandler(commandLine.withCharset(StandardCharsets.UTF_8));
try {
var handler = new OSProcessHandler(commandLine.withCharset(StandardCharsets.UTF_8));
OutputStream processInput = handler.getProcessInput();
Files.copy(ioFile.toPath(), processInput);
processInput.flush();
processInput.close();
return new FormattingTask() {
@Override
public void run() {
Expand All @@ -86,6 +83,14 @@ public void processTerminated(@NotNull ProcessEvent event) {
}
});
handler.startNotify();
try {
Files.copy(ioFile.toPath(), processInput);
processInput.flush();
processInput.close();
} catch (IOException e) {
handler.destroyProcess();
request.onError("NixIDEA", e.getMessage());
}
}

@Override
Expand All @@ -99,7 +104,7 @@ public boolean isRunUnderProgress() {
return true;
}
};
} catch (ExecutionException | IOException e) {
} catch (ExecutionException e) {
request.onError("NixIDEA", e.getMessage());
return null;
}
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/nixos/idea/lang/NixLanguage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

public class NixLanguage extends Language {
public static final NixLanguage INSTANCE = new NixLanguage();

public static final String NOTIFICATION_GROUP_ID = "NixIDEA";
public static final String NOTIFICATION_GROUP_KEY = "notification.group.nixidea";
private NixLanguage() {
super("Nix");
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/nixos/idea/settings/NixLangSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import java.util.Collections;
import java.util.Deque;

@State(name = "NixLangSettings", storages = @Storage(value = "nix-idea-tools.xml", roamingType = RoamingType.DISABLED))
@State(name = "NixLangSettings", storages = @Storage(value = "nix-idea-lang.xml", roamingType = RoamingType.DISABLED))
public final class NixLangSettings implements PersistentStateComponent<NixLangSettings.State> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. I don't want to rule out that we may add a simple built-in formatter to the plugin at some point.
  2. 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.

Copy link
Contributor Author

@cottand cottand Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Screenshot 2024-06-03 at 21 18 54

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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">
    <!-- LSP settings -->
  </component>
  <component name="NixExternalFormatterSettings">
    <!-- External formatter settings -->
  </component>
</application>

instead of

<application>
  <component name="NixLspSettings">
    <!-- LSP settings -->
  </component>
  <component name="NixLangSettings">
    <!-- Other system-dependent settings -->
  </component>
</application>

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 see, thanks for the explanation! That makes sense, I will apply your suggestion then.


// TODO: Use RoamingType.LOCAL with 2024.1
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/nixos/idea/settings/NixLangSettingsConfigurable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ public class NixLangSettingsConfigurable implements SearchableConfigurable, Conf

@Override
public @NotNull @NonNls String getId() {
return "org.nixos.idea.lsp.NixLangSettingsConfigurable";
return "org.nixos.idea.settings.NixLangSettingsConfigurable";
}

@Override
public @NlsContexts.ConfigurableName String getDisplayName() {
return "Nix Language Server (LSP)";
return "Nix";
}

@Override
Expand All @@ -57,7 +57,7 @@ public class NixLangSettingsConfigurable implements SearchableConfigurable, Conf


return FormBuilder.createFormBuilder()
.addComponent(new TitledSeparator("Formatter Configuration"))
.addComponent(new TitledSeparator("External Formatter Configuration"))
.addComponent(myTextArea)
.addComponent(myEnabled)
.addLabeledComponent("Command: ", myCommand)
Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/META-INF/plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@
instance="org.nixos.idea.settings.NixLangSettingsConfigurable"/>

<formattingService implementation="org.nixos.idea.format.NixExternalFormatter"/>

<notificationGroup id="NixIDEA" displayType="BALLOON" key="notification.group.nixidea"/>
</extensions>

</idea-plugin>