tree-sitter-python icon indicating copy to clipboard operation
tree-sitter-python copied to clipboard

My takeaways from working with this grammar

Open digitcrusher opened this issue 5 months ago • 3 comments

This is a pull request of a few fixes I had to apply while adapting this grammar for my text editor. Here's some reasoning behind each commit, from oldest to newest:

  1. The first commit slashes a dozen individual queries to match function parameter names into just one. This closes #199.
  2. Classifying assignment statements as expression statements is not only wrong (Python 3, Python 2) but also introduces an unnecessary level of indirection when querying for class attribute declarations.
  3. as_pattern already has an alias field. This change brings _as_pattern into line.
  4. Free stuff. Discriminating member identifiers is necessary to ensure lambda foo: foo == obj.foo produces the correct result. The only other solution is to capture expression as local.reference instead of identifier but that produces a lot of useless matches.
  5. Self-explanatory.

digitcrusher avatar Jul 25 '25 08:07 digitcrusher

Hi @digitcrusher,

I'm not a maintainer here, but want to thank you for your changes as they address issues I've also encountered with this grammar (when using it in https://mergiraf.org/). Let's hope it will get reviewed and merged eventually.

Classifying assignment statements as expression statements is not only wrong (Python 3, Python 2) but also introduces an unnecessary level of indirection when querying for class attribute declarations.

I'm wondering if this level of indirection is useful at all - I would be tempted to hide the expression_statement node completely, and just expose its child (be it assignment or something else). In our context this would be helpful for us to associate doc strings to the elements they document.

I've integrated some of your changes to this fork. The remaining changes are left as open PRs because of failing tests.

wetneb avatar Sep 10 '25 11:09 wetneb

lambda parameters are now valid in contexts they previously weren't

Do you mean the parentheses? I moved them out of the parameters node into function_definition/function_definition_scope. And a lambda parameter is a subset of a normal parameter, as far as I can tell, so idk.

I don't understand the addition of the definition scopes in the 4th commit (and it wasn't explained in the PR description) - if they're solely for the locals queries, I'd revert that.

The definition scopes are indeed only there, so that functions definitions would actually exist in the scope they are defined in. I've already learned from others and experienced myself that locals queries are not without their downsides and have limited them to function parameters (which don't need these new nodes) in my own fork. I could do some commit reordering to get this into this PR, if you want it.

The member identifier changes look fine if that's what it's called in the Python spec.

The member identifiers are necessary for locals queries, but other than that they are not given special attention in the language reference and are mere workarounds around the rigidness of Treesitter's query language, which greatly simplify highlighting any kind of members. Ugly but handy.

digitcrusher avatar Sep 11 '25 13:09 digitcrusher

I don't think we want to remove the node for the parameter list, which includes the parens. If you wanted to add another node for the parameters within the list, I'd be open to that.

maxbrunsfeld avatar Sep 22 '25 15:09 maxbrunsfeld