Use tree sitter for getting the node id (aka test id) for the function and class at point

[rodrigomorales1]

The master branch uses the function python-pytest--current-defun for getting the test id of the function at point or class at point and it supports at most 2 parts. The changes in this pull request adds two functions: python-pytest--node-id-def-at-point (link) and python-pytest--node-id-class-at-point (link) which get the path for the def or class at point for an arbitrary number of nested classes, these two functions use tree-sitter. See the file tests/test-python-helpers.el (link to file) for examples on how these functions behave. By having two functions for different purposes, the user has more control over what to execute (e.g. (s)he could execute a Test class when the point is on a function).

Please let me know what you think of these changes, so that I can improve this pull request to suit more use cases.

[rodrigomorales1]

I just noticed that I have used both terms "path" and "node id" in function names and docstrings for referring to the same thing. I think it would be better to use the same terminology used in the pytest documentation, that is to use "node-id" instead of "path"

Retrieved from https://docs.pytest.org/en/8.3.x/example/markers.html#node-id

Node IDs are of the form module.py::class::method or module.py::function. Node IDs control which tests are collected, so module.py::class will select all test methods on the class. Nodes are also created for each parameter of a parametrized fixture or test, so selecting a parametrized test must include the parameter value, e.g. module.py::function[param].

Retrieved from https://docs.pytest.org/en/8.3.x/example/markers.html#selecting-tests-based-on-their-node-id

You can provide one or more node IDs as positional arguments to select only specified tests. This makes it easy to select tests based on their module, class, method, or function name:

Give me a few minutes to make another commit.

[pkryger]

With a caveat that I haven't read all code changes yet, I'd like to rise one point.

I think that the change is great, and I believe using treesit is the way to go in a future. However, the package (python-pytest) is marked as Emacs-24 compatible and, IIRC, treesit has only been available since Emacs-29. Would this change work in older Emacs? Perhaps some compatibility layer would be needed (I doubt that compat has treesit, but maybe worth checking)? Alternatively, perhaps the minimum version required should be updated?

[wbolster]

same concerns here!

i'm all for using more modern and more reliable ways to do syntactical heavy lifting, but i don't want to break this package for people who are using reasonably modern operating systems (e.g. ubuntu lts).

that said, emacs 24 is essentially a random number that was perhaps relevant at some point for some compat reason, but that version is from the 2012–2015 era (release history) and i don't care at all about ancient software. (if you do, good for you, but then also use this package from that time… which didn't exist 🙃)

[pkryger]

Moving discussion about minimum required Emacs back to main thread.

... but i don't want to break this package for people who are using reasonably modern operating systems (e.g. ubuntu lts).

I agree. I didn't consider versions that don't have tree-sitter when I did the previous commits, but now I think we should consider them.

I think I could add a conditional in the transient so that it displays the new functions if the user is using GNU Emacs that supports tree-sitter (i.e. >= 29). Users that don't have tree-sitter support would continue using python-pytest--current-defun, and users that have tree-sitter would use python-pytest--node-id-def-at-point and python-pytest--node-id-class-at-point.

I think this (a compatibility layer) was something I have been trying to suggest in my original comment. The only nit pick would be - as I was once told - the recommended solution is to use some predicate (i.e., one of functionp/packagep/featurep) in such an if block. IMHO an implementation like that is slightly harder to maintain (than an explicit version check) in longer period. Especially when minimum supported Emacs version is meaningful: it requires a mental mapping between supported functionality and Emacs versions. I tend to add comments with minimal version in places like that.

Last but not least, you’d probably want to add (require treesit nil t) stanza to the beginning of the main package file.

[rodrigomorales1]

In the recently introduced commits ...

• I removed the (declare (obsolete statements because as I mentioned previously, I agree that we should not break this package for users that don't use treesit.
• I added the suffix -treesit to those functions that use treesit features. This will help us maintain the code since now we have two functions that does similar tasks but use different algorithms.
• I have added a defcustom to control whether functions that use treesit should be displayed in the transient. See https://github.com/rodrigomorales1/emacs-python-pytest/blob/a104cf85b6acf1e3f37f5faf4a9a9060d92b305e/python-pytest.el#L130-L138 and https://github.com/rodrigomorales1/emacs-python-pytest/blob/a104cf85b6acf1e3f37f5faf4a9a9060d92b305e/python-pytest.el#L191-L194

@pkryger @wbolster Please let me know what you think.

[pkryger]

It looks good to me, and (almost) ready to merge I haven't read it in great detail (and I don't think I will have time to in next few days 😟).

I think the only missing thing is (require 'treesit nil t) in the beginning of the file (around l. 32)

[pkryger]

How about

Suggested change

	(defcustom python-pytest-use-treesit nil
	(defcustom python-pytest-use-treesit (featurep 'treesit)

[wbolster]

is there any reason to NOT use treesitter even though it's available? i'd rather have less configuration and more ‘dwim’ behaviour, and not having this customizable at all would have my preference.

[rodrigomorales1]

is there any reason to NOT use treesitter even though it's available?

@wbolster There are some differences between the functions in main branch and the functions that use treesit. Users that still prefer the behavior in main branch could set the variable python-pytest-use-treesit to nil. I suggest that we have a custom variable to provide users more control on how the package works.

Difference 1: Depths of classes

In main branch, the functions python-pytest-function (renamed to python-pytest-run-def-or-class-at-point in a commit in this pull request) and python-pytest-function-dwim (renamed to python-pytest-run-def-or-class-at-point-dwim in a previous commit in this pull request) both use python-pytest--current-defun (renamed to python-pytest--node-id-def-or-class-at-point in a previous commit in this pull request).

In main branch, python-pytest--current-defun gets the current function or class at point up to a second depth. The new function python-pytest--node-id-class-at-point-treesit get the class at point for an arbitrary depth. The new function python-pytest--node-id-def-at-point-treesit gets the def at point for the outermost function (because pytest doesn't allow grouping tests using functions inside functions, it only allows using classes inside classes). See example below.

class ClassDepthOne:
  class ClassDepthTwo:
    class ClassDepthThree:
      def print_foo(self):
        def print_bar():<point>
          print("foo")

ELISP> (python-pytest--current-defun)
"ClassDepthOne::ClassDepthTwo"
ELISP> (python-pytest--node-id-class-at-point-treesit)
"ClassDepthOne::ClassDepthTwo::ClassDepthThree"
ELISP> (python-pytest--node-id-def-at-point-treesit)
"ClassDepthOne::ClassDepthTwo::ClassDepthThree::print_foo"

There are some differences between the functions with the suffix -treesit and the functions in main branch. Users that still prefer the behavior in main branch, could use

Difference 2: dwim behavior

In main branch, the function python-pytest-function-dwim search for a test file and for test names when the current file is not a test file (see below). There is no function that use treesit and implement such behavior.

emacs-python-pytest/python-pytest.el

Lines 293 to 294 in bfcd288

	file (python-pytest--sensible-test-file file)
	func (python-pytest--make-test-name func))

[rodrigomorales1]

@pkryger I think the only missing thing is (require 'treesit nil t) in the beginning of the file (around l. 32)

Done, see 1f29d55

@pkryger How about

(defcustom python-pytest-use-treesit (featurep 'treesit)

Done, see e09e8f8

[pkryger]

LGTM 🚀

[rodrigomorales1]

In the latest commit, I improved *-treesit functions so that they can correctly handle narrowed buffers. I also added tests when the buffer is narrowed.

[wbolster]

lgtm 🚀

[wbolster]

merged, thanks @rodrigomorales1 and @pkryger!