Support document formatting

[beetrootpaul]

(Disclaimer: Maybe it's just me not familiar with VS Code, since I do not use it on a daily basis 🙂 )

When I run Format Document VS Code action on a Lua file (which I #include in my p8 cart), VS Code shows me "There is no formatter for 'pico-8-lua' files installed." prompt and no formatting happens 🙁

For me, lack of document formatting is one of last reasons why I do code for PICO-8 in another IDE I am used to, even though there is no dedicated PICO-8 plugin for that IDE (which means I have to write vanilla Lua instead of the PICO-8 flavour).

---

• OS: macOS Monterey 12.6
• VS Code version: 1.72.2
• installed VS Code extensions: pico8-ls only (I don't know how to check its version, but its README embedded in VS Code has "0.4.8 (9/23/2022)" as the most recent entry in the changelog)

[japhib]

I would love to support this at some point. It's unfortunately a bit more complicated than you might think, or at least my past attempts have run into issues pretty quickly. But this would be a great feature to have.

[beetrootpaul]

I totally understand it! 🙂

Something you probably thought about already, but just in case:

• there is a luafmt-vsc plugin, which seems to format Lua files correctly in my VS Code. Repo: https://github.com/appgurueu/luafmt-vsc
• which uses luafmt as a dependency. Repo: https://github.com/appgurueu/luafmt
• and luafmt apparently is a JS code: https://github.com/appgurueu/luafmt/blob/master/index.js
• which means we could fork it and adapt to PICO-8 syntax, then take a look at luafmt-vsc to learn how to integrate it with VS Code 🤔

[japhib]

That's great info! I'll definitely take a look at that stuff as a starting point, whenever I get to this.

[japhib]

Or if you want to take a crack at it, feel free! I'm happy to provide help getting set up for developing this extension, as well as for figuring out how to set up the formatter and adapt it to PICO-8 syntax.

[beetrootpaul]

@japhib I might try. Not promising anything, since I my lose motivation on a first obstacle, especially with very little time left after hours for such topics 😅 But definitely if you are able to provide me with some good YT video or article or just short set of instructions on how to develop a VS Code plugin, it would help me a ton (I mean stuff like "I have pico8-ls from the official source installed, but how do I tell VS Code that I want to use me locally checked out fork instead" and "how do I tell VS Code to load most recent changes of that locally developed plugin?")

In case you find it easier, you can DM me on Discord. Same username as here, active a little bit on PICO-8 server among others.

[beetrootpaul]

I think I managed to setup it:

• I symlinked an extension under development into ~/.vscode/extensions/
• I modified all package.json dependencies in extensions to point to their file: local checkouts
• in VS Code I run "Reload Window" to get most recent changes
• I see errors thanks to "Developer: Show logs…", then "Extension Host"

[beetrootpaul]

one more piece of info: chain of dependencies is longer, but at least one of them seems partially covered: https://github.com/PictElm/pico8parse is a PICO-8 flavour of luaparse. I didn't manage to use it out of the box (it expects a p8 cart with all its sections instead of solely a Lua file), but it is really promising

I managed to chain most of dependencies together and see some how code format got affected by some changes I made locally.

No more updates for now, will let you know if I achieve anything Proof-of-Concept-and-git-commit worthy 🙂

[japhib]

Sounds like some good progress!

Just a note -- pico8-ls's lua parser is a heavily modified version of luaparse as well, so it may be compatible with luafmt. It's pretty different, since luaparse is in a much older style of JS that uses a lot of global variables and such, and I rewrote it in modern TypeScript, with no globals, and with classes instead of plain objects.

Ultimately, I'd like to have the formatter use pico8-ls's lua parser rather than something else, just so we don't have multiple parser implementations in the same project. So if you're able to get the formatter hooked up with regular Lua formatting, I'd be happy to work on switching out the parser implementation to use pico8-ls's existing lua parser 🙂

[beetrootpaul]

That sounds great! One of my biggest concerns from yesterday night experimenting was that it might end up with N independent tools, each one creating AST on its own way, with different set of features 😅

[beetrootpaul]

@japhib

1. What Node.js version do you use for development of this plugin?

2. Do you think it would work for you if you create a dedicated branch for formatter and I would create incremental small PRs targeted at that branch?

3. I defined some formatter (simple hardcoded text replacement) in this project successfully, but now I am stuck when trying to use luafmt instead (not PICO-8 flavour, just vanilla Lua one, which should work because my codebase is vanilla Lua, moreover it fails even on an empty text chunk). You can try it here: beetrootpaul@c3d6605 Command I use to build vsix file is npm i && npm run clean && npm run compile && npx vsce package. Whenever I created it, I install it in VS Code and perform "Developer: Reload Window". Then I see error (see comments in files modified in my repo) in "Extension Host" logs. And I see plugin doesn't activate properly, because typing map( doesn't bring docs popup for me (but it works on your most recent commit, without my changes).

[japhib]

1. I'm using Node 16, not sure it matters though because when it actually runs, it runs using VSCode's packaged version of the Node runtime
2. Sure, that would be great!

A few things on 3:

• There's an included "Launch Client" launch task in the repo (in .vscode/launch.json) -- if you use that task, it should save you from having to do npx vsce package and manually install it each time you make a change.
• I renamed the existing old formatter branch on this repo to japhib-formatter to avoid conflicts with the branch you're currently working on.
• Knowing nothing about luafmt or how it's supposed to work, I'm pretty ill equipped to troubleshoot those issues :/ I'll try to find some time soon to look into this though! (My only guess is that error.js is a missing dependency of luafmt)

[japhib]

Actually, I think I found the issue. This is what node_modules/lustils/index.js looks like:

Since the dependency on ./error.js isn't a string literal but is generated at runtime, my guess is that the bundler (esbuild in this case) isn't finding it properly.

It looks like the latest version of lustils has fixed this problem with a proper require("./error.js"): https://github.com/appgurueu/lustils/blob/master/index.js

So now the issue is to update the version of lustils that luafmt pulls in. Since luafmt consists of only a single file, probably the easiest thing to do at this point is just to pull that file into this repo, and then manually add its dependencies here as well.

Another note: on your branch it looks like you added the dependencies on package.json, however, for correctness/consistency it should probably go on client/package.json. We may want to move it onto the language server portion of it eventually (not sure of the details of how that will go) but for now, since the deps are used by client/extension.ts, I think they should go in client/package.json.

[beetrootpaul]

thanks for your answers, they already provide me with a lot of info!

Regarding a proper setup and inlining files: to not get overwhelmed in a limited time, I approach this in a veeery step-by-step way. So yes, please keep that feedback coming! I hope after some time it will all converge into a proper final solution! 🙂

I will come back with specific questions later, after I will get a chance to have another moment on this 🙂

[beetrootpaul]

Update: I just found https://code.visualstudio.com/api/language-extensions/language-server-extension-guide and it clarifies things even more for me. You know, VS Code extension dev is totally new for me, so that guide helped to understand where to run that "Launch Client" task 😄

Right now might MVP goal is to implement connection.onDocumentFormatting(…) in your server.ts, which would fix indentation only (I guess this is the simplest to implement yet still useful feature). I do not plan to support range formatting yet. Moreover I think about doing this for pico-8-lua filetype only, since sounds easier (and is what I need this whole extension for in the first place, my reason to try to do it at all 😄 ).

I also wonder what issues did you have with your japhib-formatter branch, if possible to explain in a simple way? I see a lot of code there which, I suppose, it the way you intended it to be. I think I might take a look at your formatter.ts and try to use some part from it to implement indentation formatting? I guess using external tools like luafmt would only get in the way, not helping at all? Just wondering, I only skimmed through your formatter branch so far.

What do you think in general @japhib ?

[beetrootpaul]

@japhib Apart from the post above, I also created a draft PR #27 to move discussion there and to see up-to-date code easily.

[japhib]

I also wonder what issues did you have with your japhib-formatter branch, if possible to explain in a simple way?

To be honest, it's been long enough that I don't remember the specific issues. I think in general there were just a lot of little issues, and it was lower priority for me at the time, so I decided to put it off.

I think in general basing it off of the existing japhib-formatter is probably a good idea since it's already well integrated with the existing parser and pattern of doing things.

The formatter.ts in that branch uses the Visitor interface which I suspect will need some bigger structural changes to get working properly; for example, having to insert the comments like luafmt does is probably something bigger/more complex than the current Visitor code can handle. The existing Visitor stuff is more suited to "regular" language server stuff like finding document symbols and definitions/usages, and the formatter usage may be different enough to warrant a whole new thing, rather than a bunch of big edits to Visitor which seems like it will become increasingly harder to reconcile with the symbols/definitions pattern of usage.

[beetrootpaul]

@japhib

for example, having to insert the comments like luafmt does is probably something bigger/more complex than the current Visitor code can handle

quick thought: would it be wrong to include comments in parsed AST so they could be inserted as any other statement with let's say this.visitCommentStatement(node)?

(update: I read somewhere that it is then no longer AST, but FST – Full Syntax Tree. Not sure if such approach is fundamentally different for this extension, just thinking loudly)

[japhib]

I had some time today so I went ahead and took a stab at some of the problems you're facing.

I'm not sure how best to commit exactly to your branch, since your branch is a fork of mine, so for now I figure we can just cherry-pick commits back and forth as needed. My branch is called brp-formatter and is based on your PR branch. Here's the commit I made today: 3aae15c

Note that a bunch of the changes are just formatter changes. I have my editor format on save so it's semi-unavoidable. If you want the same behavior you can install the ESLint VSCode extension, and it should format all the files you work on according to the formatting rules defined for this project in .eslintrc.js.

Here's my progress:

1. I was able to get comments inserted into the AST! So now they are preserved when formatting the file.
2. I fixed the error where local a would get transformed into local a =
3. I fixed the error where not a would get transformed into nota

Still needs work:

1. Whitespace is not preserved. I added a Whitespace node type that can be used in the future, but it's unused for now.
2. Comments inside of statements are handled wrong:

long_fun(
  -- first arg
  abc,
  -- second arg
  def
)

gets transformed to this:

long_fun(abc, def)
-- first arg
-- second arg

So, that definitely needs some work.

Anyways, try it out and let me know what you think! Please continue to point out any specific cases where it does weird things, and I'll be sure to fix those and include them in testing.

(Btw, you don't have to ping me on each comment - I receive a notification for every comment on this thread anyway, I believe because I commented once and now GitHub knows I'm following it. 🙂)

[japhib]

I also eventually want the formatter to be smart about preserving the original author's style.

Part of that would be the ability to customize how the formatter works, i.e. specifying an option of whether you prefer spaces around your arithmetic operators like a+1 or a + 1.

Another part of that would be detecting when something like a table literal is in one single line and not expanding it to multiple lines. For example, this:

this.dash_target={x=0,y=0}
this.dash_accel={x=0,y=0}
this.hitbox = {x=1,y=3,w=6,h=5}

gets expanded to this:

this.dash_target = {
  x = 0,
  y = 0
}
this.dash_accel = {
  x = 0,
  y = 0
}
this.hitbox = {
  x = 1,
  y = 3,
  w = 6,
  h = 5
}

which reads totally different and feels like it shouldn't have been changed so drastically.

Anyways, not totally sure how I want to approach that yet, but something to keep in mind.

[beetrootpaul]

(answered in PR since the previous intention to move discussion there didn't really happen 😄 👉 #27 (comment) )

(but if you prefer to discuss here in the issues, just let me know 🙂 I just assumed it is more native to GitHub's design to discuss implementation detail in PR, but I have no real open source experience, just commercial ones, where discussion usually happens elsewhere outside GitHub)

[beetrootpaul]

Separate but related question: what do you think about externalising parser and formatter to their own packages at some point in the future?

Why am I asking? Because now, when I started to believe my transition to VS Code for PICO-8 code might really happen (due to ongoing work on formatter), I realised there is one more piece of this puzzle for me to transition completely. And that is… minifier. Right now I use https://github.com/beetrootpaul/luamin (my slightly modified fork of luamin). The main problem I have with luamin is that it doesn't support PICO-8 syntax.

Therefore why I think having an access to pico8-ls parser could help to build other tools using it, for example a custom minifier 🙂

[japhib]

I would support that. I was mainly waiting for a legitimate use case (such as the minifier, as you mention) to come along. Do you mind opening another issue to track work on that? Go ahead and include all the info/suggestions/use cases from this comment.

[beetrootpaul]

OK 🙂 👉 #29

[japhib]

Already fixed with 0.5.0