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.

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

Jump to bottom

fix(topiary-queries/scm): Fixed invalid formatting of field_definitions #826

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix(topiary-queries/scm): Fixed invalid formatting of field_definitions #826

wants to merge 9 commits into from

Conversation

mkatychev
Copy link

@mkatychev mkatychev commented Jan 9, 2025 •
edited
Loading

Fixed invalid formatting of field_definitions

Description

  • prevent invalid formatting of field_definition that followed an identifier 
    (function_call function: (identifier) @function)
    Currently the line above would format to the invalid output below: 
    (function_callfunction: (identifier) @function)
  • predicates parameters are now separated by a spaced_softline allowing long predicates to be broken up 
    (
  (identifier) @function.builtin
  (#any-of?
  @function.builtin
  "union"
  "difference"
  "intersection"
  "circle"
  "square"
  "polygon"
  "text"
  "import"
  "projection"
  "sphere"
  "cube"
  "cylinder"
  "polyhedron"
  "linear_extrude"
  "rotate_extrude"
  "surface"
  "translate"
  "rotate"
  "scale"
  "resize"
  "mirror"
  "multmatrix"
  "color"
  "offset"
  "hull"
  "minkowski")
)
    Currently the code would be formatted into a very long predicate line: 
    (
  (identifier) @function.builtin
  (#any-of? @function.builtin "union" "difference" "intersection" "circle" "square" "polygon" "text" "import" "projection" "sphere" "cube" "cylinder" "polyhedron" "linear_extrude" "rotate_extrude" "surface" "translate" "rotate" "scale" "resize" "mirror" "multmatrix" "color" "offset" "hull" "minkowski")
)

Checklist

Checklist before merging:

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

@mkatychev
Copy link
Author

I wasn't sure where to put these two test cases as topiary-cli/tests/samples/input/tree_sitter_query.scm seems to be a carbon copy of the ocaml query

@mkatychev mkatychev marked this pull request as ready for review January 9, 2025 19:19
@nbacquey
Copy link
Member

nbacquey commented Jan 9, 2025

The fact that topiary-cli/tests/samples/input/tree_sitter_query.scm is a de facto copy of the OCaml query file is because it was our biggest query file at the time. You're more than welcome to add your test cases to it: the file is never actually used to format OCaml code.

@mkatychev
Copy link
Author

mkatychev commented Jan 9, 2025 •
edited
Loading

The fact that topiary-cli/tests/samples/input/tree_sitter_query.scm is a de facto copy of the OCaml query file is because it was our biggest query file at the time. You're more than welcome to add your test cases to it: the file is never actually used to format OCaml code.

Ok, should I just add test cases to it arbitrarily. it may be better to expand test cases into their own files and have the test suite use a set of files.
I suppose for now I can keep it in a single file and add corpus style comment headers do delimit various cases.

@mkatychev
Copy link
Author

I also see that topiary formats the tree-sitter s-expressions as

(foo
  (bar)
)

instead of the trailing parentheses I see everywhere else:

(foo
  (bar))

Is that intentional?

@Xophmeister
Copy link
Member

I think this was just the style we adopted at the time and it's stuck. There was no particular reason, as far as I remember.

Xophmeister
Xophmeister requested changes Jan 10, 2025
View reviewed changes
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.

Thank you for your contribution 🙏

I've made a few suggestions and I have a couple of more general comments, from the PR discussion:

  1. It would be great if you could add test cases to topiary-cli/tests/samples/{input,expected}/tree_sitter_query.scm. Feel free to add them arbitrarily; it doesn't matter that it diverges from our OCaml rules.

  2. For the time being, I think we should stick with the Algol-style indentation, rather than Lisp-style. That is:

    ; Algol-style
(foo
  (bar
    (quux)
  )
)

; Lisp-style
(foo
  (bar
    (quux)))

    I think there's a strong argument to switch to Lisp-style, but I think that warrants an issue -- feel free to create one -- so the Topiary Team can consider it.

CHANGELOG.md Outdated Show resolved Hide resolved
topiary-queries/queries/tree_sitter_query.scm Outdated Show resolved Hide resolved
@mkatychev mkatychev mentioned this pull request Jan 11, 2025
Open
@mkatychev mkatychev requested a review from Xophmeister January 11, 2025 04:54
mkatychev
mkatychev commented Jan 11, 2025
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
Xophmeister
Xophmeister requested changes Jan 13, 2025
View reviewed changes
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.

Thanks for your changes 🙏

This is very close to being correct, however there's a small bug when multiple captures/identifiers appear after the predicate in a multi-line context.

topiary-queries/queries/tree_sitter_query.scm Outdated Show resolved Hide resolved
Xophmeister
Xophmeister requested changes Jan 15, 2025
View reviewed changes
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.

Apologies; looks like I was too quick to approve!

There is one more edge case, that the CI uncovered. When a predicate has nothing after it (captures, identifiers or strings), then an errant space ends up in the output:

    (tag_specification) @prepend_delimiter
    (#delimiter! "| ") ; sic
-   (#multi_line_only!)
+   (#multi_line_only! )
  )

@Xophmeister
Copy link
Member

@mkatychev It's an easy fix; I'd push it myself, but I doubt I have appropriate access to your fork. This rule is both redundant and adding the errant space when there are no parameters to the predicate:

(predicate
  (predicate_type) @append_space
)

It can simply be removed 👍

@mkatychev
Copy link
Author

mkatychev commented Jan 15, 2025 •
edited
Loading

@Xophmeister I've definitely had maintainers push directly to my branches before.

P.S.
Did this GHA job really run for 24 minutes?
EDIT: looks like part of it is LALRPOP building

mkatychev and others added 9 commits January 15, 2025 12:03
@mkatychev
tree_sitter_query changes:
5db2a43 
* prevent invalid formatting of `field_definition` that followed an
  `identifier`
* `predicates` parameters are now separated by a `spaced_softline`
  allowing long predicates to be broken up
@mkatychev
added to changelog
1ee05ab
@mkatychev @Xophmeister
Update CHANGELOG.md
6e1ebfe 
Co-authored-by: Christopher Harrison <Xophmeister@users.noreply.github.com>
@mkatychev
handle indentation of predicate parameters
a3e3201
@mkatychev
fixup expected header
0f73718
@mkatychev
handle predicate_type spacing
a7536dc
@mkatychev
Update CHANGELOG.md
8f88b7b
@mkatychev
PR feedback addressed on multi tag predicate scenario
b17e726
@mkatychev
fix(query/scm): removed append_space after predicate type
9c5abf6
@mkatychev mkatychev force-pushed the fix/ts-query-formatting branch from b670b35 to 9c5abf6 Compare January 15, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xophmeister Xophmeister Xophmeister requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkatychev @nbacquey @Xophmeister