Maarti / JenkinsDocExtension

Extension for VS Code providing Jenkins docs
GNU General Public License v3.0
12 stars 2 forks source link

Search for definitions in all *.groovy files in workspace in src/go-definition-provider.ts #28

Closed benmdavidson closed 3 years ago

benmdavidson commented 3 years ago

Can you refactor the definition provider function such that it searches all groovy files in workspace for the selected function definition? I'd do it myself and PR but I don't know what I'm doing with vscode extensions or TS in general.

This would be useful because nearly every function call in my groovy project is defined in a different file (not named by the function, which is what your current implementation is searching for).

Maarti commented 3 years ago

Hi, thank you for your feedback. Do you have a short example of how is declared your function (and the filename) and how is it called?

Is the opening curly brace of your function on the same line as this one? (because for information, the "go to" does not currently work if the brace is on a new line because the regex searches line by line)

And your file is not named by the function, but your function call is appended to the filename, isn't it? (like myFile.myFunction())

Because I thought we had only 2 possibilities with Jenkins:

But I'm not a Jenkins expert so if you are in any other situation i would be curious about that 😃

goto

benmdavidson commented 3 years ago

Here is an example of the function I have (I cannot give the exact example because the code I'm working with is not open source.) The filename is arbitrary and unrelated to the function name.

Integer fun (Integer param1,
             Integer param2,
             Integer param3) {

                 // Function body

             }

fun();

If you try to Go-to Definition it fails because the the opening curly brace { is unmatched on the first line. I implemented a fix for this specific issue (see PR #31 ) by truncating the end of the regex at the opening parenthesis. The only thing I'm unsure about is if this conflicts with some other case, although I can't imagine it does.

I think the issue is that you believe you must always call a named function with myFile.myFunc() , which is not the case if you call that function within the same file, where you can simply call myFunc() and it will imply that the function is defined locally in that file.

Maarti commented 3 years ago

Thanks for the example. The "GoToDefinition" for methods in the same file is already implemented, but all "GoTo" currently only work if the method signature is on the same line (the method, parameters and the opening curly brace) That is why it doesn't work in your case, because your parameters are multiple lines.

I just fixed it and published the version 1.6.0. I changed the regex to now work for any number of lines.

Here is a test with your example:

goto2

benmdavidson commented 3 years ago

Thank you for the feedback and for making this change, I learned from how you implemented the changes. The Go-to works for multiline method signatures as you demonstrated, thanks a lot!

There is still a problem though. I noticed you rejected the change where I modified the file pattern to be "*/.groovy". Is there any specific reason you have for not accepting this change?

The current implementation for Go-to for methods called on objects does not work. I will explain why with an example.

Note that groovy is a superset of Java, thus any Java program is valid in groovy, including classes, objects, etc.

// File name is arbitrary

class PipelineClass {

    private Integer local_int

    PipelineClass(Integer local_int) {
        this.local_int = local_int
    }

    Integer getInt () {
        return this.local_int
    }

}

pipelineInstance = new PipelineClass(4)
Integer value = pipelineInstance.getInt()  // THIS LINE HERE

return this

The error is on THIS LINE HERE, Go-to Definition does not find the definition of "getInt" because it is searching for a file called "pipelineInstance" which does not exist. There is no efficient or elegant way I can think to do this. The change from PR #31 where the pattern is changed to "*/.groovy" solves this problem.

If you are concerned with this change because of performance, my project is enterprise scale with >100 directories nested 3-4 levels with 100s of groovy files and found the definition in under 1 second with the fix when tested locally.

This functionality is quite necessary for my use-case considering most function calls within this code base are made on objects.

Please let me know if this description or suggestion is unclear and I'll be happy to offer more information.

Maarti commented 3 years ago

Ah yes of course, I had not thought of that because I did not have this type of case on my project. I will add your suggestion as soon as possible.