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

feat(query): Add OpenSCAD formatter #845

Merged
merged 48 commits into from
Jan 29, 2025
Merged

Conversation

mkatychev
Copy link
Contributor

@mkatychev mkatychev commented Jan 21, 2025

feat(query): Add OpenSCAD formatter

Description

My work on supporting alternate formatters for openscad-lsp exposed a lack of idiomatic formatting for the OpenSCAD DSL with clang falling somewhat short of expectations.

The intent of this PR is to build a formatter that is as close as possible to idioms found in popular OpenSCAD libraries such as BOSL2 and standard library.

Cheatsheet for ref of grammar types.

This PR uses a fork of tree-sitter-openscad since the main repo has been
inactive for some time: bollian/tree-sitter-openscad#18

grammar.source.git = {
git = "https://github.com/mkatychev/tree-sitter-openscad.git",
rev = "1b3e5fd00245c097f501532611f349c88d5b334b",
},

The changes done to tree-sitter-openscad have been quite large so there has
been at ton of grammar changes behind the scenes to allow these formatting queries to behave idiomatically:
https://github.com/mkatychev/tree-sitter-openscad/blob/master/CHANGELOG.md#version-060

I've done my best to document most of the queries and test in this PR but I'm sure there are things I've missed.

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

@nbacquey
Copy link
Member

The code looks fine at first glance, but it seems your Rust tool chain has bumped the Cargo Lock version from 3 to 4, which our CI does not support. Have you built your branch with the dev environment provided by Nix?

Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

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

This looks great 🙏

I've left one minor comment and, as per @nbacquey's suggestion, we need the Cargo.lock fixed so we can run our CI.

topiary-config/languages_nix.ncl Show resolved Hide resolved
@mkatychev
Copy link
Contributor Author

mkatychev commented Jan 22, 2025

The code looks fine at first glance, but it seems your Rust tool chain has bumped the Cargo Lock version from 3 to 4, which our CI does not support. Have you built your branch with the dev environment provided by Nix?

I have not, should nix flake check suffice or should I follow the steps in the pre-commit-hook, I think on macOS I will start with nix develop. I have a PC running NixOS if that doesn't work out.

entry = "${lib.getExe self.packages.${system}.topiary-cli} fmt";

Would be nice to add step in the flake that does a CI compliant cargo update.

@nbacquey running cargo update inside of nix develop still seems to bump version to 4, is there a more reliable way to update the lockfile?

@mkatychev
Copy link
Contributor Author

mkatychev commented Jan 22, 2025

As an aside, it seems like the CI hasn't been getting cache hits in cachix OR GHA cache, is this a known issue?

EDIT: I don't know what's failing in CI currently because the log is massive, opening the raw log causes the given firefox tab to crash :(
It may be a good idea to scale the verbosity here down to -vv, current output produces a lot of data:

@mkatychev mkatychev changed the title Add OpenSCAD formatter feature(query): Add OpenSCAD formatter Jan 22, 2025
@mkatychev mkatychev changed the title feature(query): Add OpenSCAD formatter feat(query): Add OpenSCAD formatter Jan 22, 2025
@Xophmeister
Copy link
Member

The code looks fine at first glance, but it seems your Rust tool chain has bumped the Cargo Lock version from 3 to 4, which our CI does not support. Have you built your branch with the dev environment provided by Nix?

I have not, should nix flake check suffice or should I follow the steps in the pre-commit-hook, I think on macOS I will start with nix develop. I have a PC running NixOS if that doesn't work out.

entry = "${lib.getExe self.packages.${system}.topiary-cli} fmt";

Would be nice to add step in the flake that does a CI compliant cargo update.

@nbacquey running cargo update inside of nix develop still seems to bump version to 4, is there a more reliable way to update the lockfile?

If I run cargo update inside nix develop, on NixOS, it doesn't update the lockfile version against the main repo. However, if I do it from your fork, then it does update the lockfile version to v4. Therefore, the reason the toolchain has shifted is because of the nix flake update.

I'm able to prevent the Cargo.lock file from getting its version bumped if I revert e5324bc and limit the flake.lock changes to:

diff --git a/flake.lock b/flake.lock
index 0e645f9..118bdf2 100644
--- a/flake.lock
+++ b/flake.lock
@@ -92,7 +92,8 @@
         "crane": "crane",
         "nixpkgs": "nixpkgs",
         "rust-overlay": "rust-overlay",
-        "tree-sitter-nickel": "tree-sitter-nickel"
+        "tree-sitter-nickel": "tree-sitter-nickel",
+        "tree-sitter-openscad": "tree-sitter-openscad"
       }
     },
     "rust-overlay": {
@@ -148,6 +149,26 @@
         "repo": "tree-sitter-nickel",
         "type": "github"
       }
+    },
+    "tree-sitter-openscad": {
+      "inputs": {
+        "nixpkgs": [
+          "nixpkgs"
+        ]
+      },
+      "locked": {
+        "lastModified": 1737584789,
+        "narHash": "sha256-j/DordcjrrVZ9ZI2p46z4TULyf2A+4mNjcM9btowyQU=",
+        "owner": "mkatychev",
+        "repo": "tree-sitter-openscad",
+        "rev": "270e5ff749edfacc84a6e4a434abd4e8b0f70bbe",
+        "type": "github"
+      },
+      "original": {
+        "owner": "mkatychev",
+        "repo": "tree-sitter-openscad",
+        "type": "github"
+      }
     }
   },
   "root": "root",

I'm no Nix expert by any stretch of the imagination, but hopefully that fixes the problem in your branch 🤞

@Xophmeister
Copy link
Member

EDIT: I don't know what's failing in CI currently because the log is massive, opening the raw log causes the given firefox tab to crash :(
It may be a good idea to scale the verbosity here down to -vv, current output produces a lot of data:

In the GitHub Actions view, there's a button on the top-right of the pane that allows you to download the log, rather than viewing it in your browser. Does that work for you?

@Xophmeister
Copy link
Member

#849 bumps the toolchain, so once that's merged, I think that will resolve the Cargo lockfile/CI issue at the source. You may want to rebase once that's merged.

@Xophmeister
Copy link
Member

#849 bumps the toolchain, so once that's merged, I think that will resolve the Cargo lockfile/CI issue at the source. You may want to rebase once that's merged.

@mkatychev #849 has now been merged

@mkatychev mkatychev requested a review from Xophmeister January 27, 2025 22:56
Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

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

This looks like it's all ready, now! Thank you, @mkatychev 🙏

I did notice a double-indent in your expected output. I don't know OpenSCAD, so maybe this is genuinely expected, but I thought I'd flag it before merging...

@Xophmeister Xophmeister merged commit b868a98 into tweag:main Jan 29, 2025
5 checks passed
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.

3 participants