✨ Add XMLPublicID condition to builtin provider #306
Conversation
Adding XML-like public-id condition supporting regular expression evaluation on the public-id attribute. Related to konveyor/windup-shim#87 Related to konveyor#221 Signed-off-by: Marek Aufart <[email protected]>
aufi
changed the title
|
@aufi is this ready for review? |
Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Marek Aufart <[email protected]>
aufi
changed the title
Signed-off-by: Marek Aufart <[email protected]>
|
Thanks for review&feedback @shawn-hurley, updated. |
|
The CI failure comes from missing update/merge of konveyor/go-konveyor-tests#65. This PR will be merged when gets green, but I'd be grateful for review. |
|
@aufi the code looks good Could you add a test in the demo to show it working so we don't break it in the future? |
Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Marek Aufart <[email protected]>
|
@shawn-hurley @jmle Added demo rule for the xmlPublicID condition. Ready for review. |
| @@ -350,6 +369,47 @@ func findFilesMatchingPattern(root, pattern string) ([]string, error) { | |||
| return matches, err | |||
| } | |||
|
|
|||
| func findXMLFiles(baseLocation string, filePaths []string) (xmlFiles []string, err error) { | |||
There was a problem hiding this comment.
FYI @aufi, in the future, unless this is used by other functions, I don't know if this extra layer for functions is necessary. It doesn't hurt, but does add a layer to reading what the main function does.
I don't think we need to change or anything, but I'm just giving my perspective and wondering what your thoughts are. I think, for now, it is perfectly fine.
There was a problem hiding this comment.
Thanks, your comment makes a good sense to me. The reason for this function was to share the function for XML&XML PublicID conditions (requested in one of previous reviews).
| return | ||
| } | ||
|
|
||
| func queryXMLFile(filePath string, query *xpath.Expr) (nodes []*xmlquery.Node, err error) { |
There was a problem hiding this comment.
Another one of the small things is that we don't have to change for this PR, but we usually don't use naked returns for a function that has some complexity.
In this case, because of the err != nil w/ checking the err on line 390, I think all the shadowing will add complexity and make it slightly harder to reason about. I don't think we have to change this, but I want to point it out.
I love to hear thoughts if you disagree just to hear a different perspective; for now, just leave it as is, and we can always address in a follow up :)
There was a problem hiding this comment.
Updated returns to use values directly (I didn't realize it is not used in analyzer in that way).
I'd plan to address mentioned issues when removing TODOs from golang XML parsing from the code.
Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Marek Aufart <[email protected]>
Adding XML-like public-id condition supporting regular expression evaluation on the public-id attribute.
Related to konveyor/windup-shim#87
Fixes: #221
Analyzer PR: 306
Windup-shim PR: 87