Add GoToLine Recipe + Utility Methods #3829
Conversation
|
Supersedes openrewrite/rewrite-analysis#41 |
JLLeitschuh
force-pushed
the
feat/JLL/go-to-line
branch
from
17dd884
to
5c7e9d6
Compare
|
@JLLeitschuh I am surprised that we provide a recipe for this. What is the usefulness of that? Especially since the recipe doesn't have a path option for the file. Would it not make more sense to only have a visitor? |
I figure if you want a path option that should, likely, be it's own recipe that flags the file as a precondition, then this is applied. |
I was implying that this should be a recipe from this comment. But I guess it was never explicitly stated by @jkschneider. Idk. I thought it may be useful to demonstrate the capability, but I'm not attached to keeping the functionality in a recipe. The recipe needs to exist in some form for unit testing though. If it's not a recipe, where would the API live? I had hoped on |
|
But when would you be using this recipe as is? I thought your use case would be programmatic use. How would you be using this? Having line and column number options but no path option seems kind of weird. |
|
For unit testing you can just use the |
|
I think we should move this code into the core as soon as we have agreed on how this services API should look like. That has now also been incubating in rewrite-java for some time. |
Quick and dirty testing on the SaaS. But in general, not often. It doesn't need to be a recipe. I'd like a suggestion on where the API should live though. |
|
I think the visitor might be interesting as a separate utility. If we additionally want a recipe, I think we should also have a path option. Apart from that I think the code is in the right place. The service is something we would build on top. |
|
|
||
| import java.util.*; | ||
|
|
||
| @Incubating(since = "8.11.3") |
There was a problem hiding this comment.
| @Incubating(since = "8.11.3") | |
| @Incubating(since = "8.12.1") |
There was a problem hiding this comment.
Resolved! Much appreciated!
|
|
||
| @Incubating(since = "8.11.3") | ||
| public boolean isPrefix() { | ||
| return this.toString().endsWith("_PREFIX"); |
There was a problem hiding this comment.
Do we get a lot of false positives without this?
There was a problem hiding this comment.
The logic does not work correctly without a check to see if the Location is a prefix.
Without it, we end up findings AST elements on other lines
Adds a recipe that will find the LST nodes in the tree at a given line/column. This is useful for matching the OpenRewrite LST against outputs from other scanners, for example security scanners that generate SARIF files. Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
JLLeitschuh
force-pushed
the
feat/JLL/go-to-line
branch
from
5c7e9d6
to
3f20a71
Compare
|
@knutwannheden PTAL at the changes I made yesterday and let me know if they are good for you to merge |
|
@timtebeek anything that can be done to get this over the finish line? |
|
Hi @JLLeitschuh ! Thanks for proposing to add this upstream; after some more discussion we feel this would fit better in rewrite-analysis or rewrite-java-security after all. Sorry about any rework; I know you started out there originally. Look forward to seeing it there, and what you'll build once it's available. |
|
Much appreciated. Any qualms on accepting the changes to |
Given the coordination required for any changes to tree packages, and our general lean towards a minimal public API it might be best to keep the pre/suffix checks within the GoToLine utility too, at least for now. Thanks again! |
|
Fair enough. Especially because, as of currently, this is likely fairly niche for now. @knutwannheden do you have insights into how stable the |
Adds a recipe that will find the LST nodes in the tree at a given
line/column.
This is useful for matching the OpenRewrite LST against outputs from
other scanners, for example security scanners that generate SARIF files.
Signed-off-by: Jonathan Leitschuh Jonathan.Leitschuh@gmail.com
What's changed?
Added a
GoToLinerecipe.What's your motivation?
Being able to find LST nodes by their line/column coordinates can be helpful when matcing up the LST against scanners.
Anything in particular you'd like reviewers to focus on?
Are there any unit tests that I'm missing that I should really make sure I include?
This strategy, unfortunately, doesn't inherintly support other languages. Is there any strategy that would make this multi-language?
Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
Any additional context
Checklist