mooz / js2-mode

Improved JavaScript editing mode for GNU Emacs
GNU General Public License v3.0
1.33k stars 186 forks source link

Imenu support for mocha-like test files #575

Closed DamienCassou closed 3 years ago

DamienCassou commented 3 years ago

I've created js2-imenu-mocha which adds describe() and it() blocks to imenu entries. Would it make sense to include that in js2-mode's repository directly? Maybe as part of js2-imenu-extras?

dgutov commented 3 years ago

Hi Damien,

Sure, why not? Does it fit the approach used in there?

DamienCassou commented 3 years ago

Does it fit the approach used in there?

@dgutov it doesn't seem like it as js2-imenu-extras.el relies on regexps. Do you have any suggestion?

dgutov commented 3 years ago

@DamienCassou The idea to use regexps was to reduce the performance impact. Especially of "styles" which are not used in the current buffer (we enable all of them by default after all). A regexp search is fast; an AST walk is slower.

For comparison, js2-imenu-walk-ast, in a medium-size file (500 lines), takes 20ms here, and 50ms when including the garbage collection. Take 10 calls like that - and you have a 200ms delay.

Have you looked into using the same implementation style? Do you think it would limit you on features?

DamienCassou commented 3 years ago

I have started investigating with this entry in js2-imenu-extension-styles:

(:framework mocha
        :call-re "^\\s-*describe\\s-*("
        :recorder js2-imenu-record-mocha-describe)

and this function:

(defun js2-imenu-record-mocha-describe ()
  (save-excursion
    (save-match-data
      (let* ((node (js2-node-at-point (1- (point))))
             (args (js2-call-node-args node))
             (name (cl-first args))
             (body (car (last args))))
        (when (js2-string-node-p name)
          (let ((name-value (js2-string-node-value name)))
            (js2-record-imenu-entry
             body
             (list name-value)
             (js2-node-abs-pos body))))))))

The problem with this approach is that the regexp matches all describe blocks, not just the top-level one. This means that the hierarchy of describe blocks it not reflected in imenu.

Here is an example. Given this JS:

describe("top-level", () => {
  describe("sub", () => { ... });
  ...
});

I would like to get this imenu entry consisting of 1 sub-entry:

(("top-level"
  ("*declaration*" . 1)
  ("sub"
   ("*declaration*" . 2")
   ...) ; content of the sub-describe
  ...) ; more content of the top-level describe

But the regexp-based solution treats all describe blocks the same. I could adapt the regexp to only match describe in the first column and that would solve most of the cases but that's not perfect.

UwUnyaa commented 3 years ago

The idea to use regexps was to reduce the performance impact. Especially of "styles" which are not used in the current buffer (we enable all of them by default after all). A regexp search is fast; an AST walk is slower.

Wouldn't providing this as an optional opt-in feature entirely sidestep the problem? There are many ways of writing test files, so some people might not be interested in such solution at all. I don't use any imenu stuff myself.

DamienCassou commented 3 years ago

Wouldn't providing this as an optional opt-in feature entirely sidestep the problem?

if you don't load js2-imenu-extras.el, you won't be impacted at all.

dgutov commented 3 years ago

@DamienCassou As the first step in js2-imenu-record-mocha-describe, you could verify that the block is the top-level one.

That's the general idea: match what is possible to match with the regexp, and then verify the information using the AST.

DamienCassou commented 3 years ago

Closing this in favor of #576.