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

Add formatting command and "format on save" #21

Merged
merged 2 commits into from Aug 3, 2021

Conversation

edwardloveall
Copy link
Contributor

I'm fresh off adding a formatter to my crystal-lang extension so I figured I'd take a try at adding a formatter for this.

{
"title": "Format Elm",
"command": "elm.format",
"shortcut": "opt-shift-f",
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've seen this used by a couple of extensions now. Happy to change it.

@@ -34,6 +34,7 @@
"@types/nova-editor-node": "^4.1.3",
"@typescript-eslint/eslint-plugin": "^4.28.5",
"@typescript-eslint/parser": "^4.28.5",
"elm-format": "^0.8.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To test formatting locally.

Comment on lines +11 to +21
const config = nova.workspace.config;
const defaultProcess = new Process("elm-format");

const configPath = config.get(`${identifier}.elmFormatPath`, "string");
if (!configPath || configPath.trim() === "") {
const message =
"Please provide an elm-format executable in Project Settings to enable formatting.";
reject(message);
return defaultProcess;
}
const elmFormatPath = nova.path.expanduser(configPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's definitely a more robust version of this which we could switch to if you want more automatic path finding (based on the node-modules path, for example) but I started here first.

return new Promise<string>((resolve, reject) => {
try {
const process = setUpProcess(reject);
let buffer: Buffer = { stdout: "", stderr: "" };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two string arrays would also be fine if this feels to weird.

Comment on lines +33 to +35
const config = nova.workspace.config;
const formatOnSave = config.get(`${identifier}.formatOnSave`, "boolean");
if (formatOnSave === true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I'm assuming some things here for convenience.

};

const writeToStdin = (process: Process, inputText: string) => {
const writer = (process.stdin as any).getWriter();
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 could not, for the life of me, get around this. A WriteableStream is typed as unknown so I had to just convert it to any 😞

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I've been stuck on these crappy *Stream types exported by Nova. On my Nix extension, I had to format the file on disk rather than in-memory. Less than ideal but I figured something now is better than nothing given that it could easily be updated later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah gotcha. When I tried that it moved the cursor to the end of the document which was frustrating. Hopefully that's avoidable.

@rhoskal
Copy link
Owner

rhoskal commented Aug 2, 2021

@edwardloveall Thanks a ton for this! I'll take a look later tonight. I've been in the middle of a branch getting ElmLS setup so hopefully that will land soon.

@rhoskal
Copy link
Owner

rhoskal commented Aug 2, 2021

@edwardloveall I'll take the code as-is. You'll need to run make build and commit that Scipts/* output for CI to be happy.

@edwardloveall
Copy link
Contributor Author

Thanks @hansjhoffman! I pushed the build files.

@rhoskal
Copy link
Owner

rhoskal commented Aug 3, 2021

@edwardloveall Before I merge this, I just want to double check that is isn't introducing the problem where the Nova editor shows the document as dirty after the editor.edit() call. I ran into that the other day and have no idea how to fix it (at this point).

@edwardloveall
Copy link
Contributor Author

Funny you should ask because this is what got me onto this pattern (with the returning promises) in the first place. It does not because if you return a Promise to TextEditor's onWillSave method Nova will wait at least 5 seconds while the formatter runs. Once resolve is called on the Promise, the document saves. The document is dirty if you don't return a promise because Nova saving the file and elm-format are racing.

I wrote up more about that here

@rhoskal
Copy link
Owner

rhoskal commented Aug 3, 2021

Funny you should ask because this is what got me onto this pattern (with the returning promises) in the first place. It does not because if you return a Promise to TextEditor's onWillSave method Nova will wait at least 5 seconds while the formatter runs. Once resolve is called on the Promise, the document saves. The document is dirty if you don't return a promise because Nova saving the file and elm-format are racing.

I wrote up more about that here

Awesome. I missed that part of the type signature onWillSave(callback: (textEditor: TextEditor) => void | Promise<void>): Disposable; where a Promise is allowed. Dang I feel stupid lol.

Thank you so much for your contribution and teaching me! I will merge your code and push a new release which should be available tomorrow morning at the latest. If you find any bugs or have any feature requests please let me know. Also, should you ever need help on any of your extension work and would like a second set of eyes, just ping me.

@rhoskal rhoskal merged commit fde6f5d into rhoskal:master Aug 3, 2021
@rhoskal rhoskal mentioned this pull request Aug 3, 2021
@edwardloveall edwardloveall deleted the el-formatter branch November 22, 2023 19:52
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.

None yet

2 participants