feat: improved error message when pom dependency version not found #253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm not sure how useful this change is? shouldn't you be able to figure out the pom based on the dependency name and the property name that is being accessed? I'm not a heavy Java user, so would appreciate if you could expand on the situation you're dealing with where the current warning is not sufficient. (also @oliverchang or @another-rex could you approve the CI workflows to run?) |
I guess it depends on how the library is being used. I came to this from scorecard, and there for example you would currently see: It's not super obvious those messages have anything to do with |
|
(on an aside, I am annoyed I forgot to add the Yeah I agree that output is not the best, though I think the problem is more with these outputs being done inefficiently because at the time I didn't want to commit to a specific way to surface them given it would impact the library function signatures and be just a whole "thing"... (I've explored a solution to this in #186 - any thoughts you'd have would be appreciated!) My concern with this change is that we're now expecting and promising more info out of the file, and that typically means more cases to handle (i.e. what happens if it's not there) - however I assume these two properties are always present in a |
:)
Yeah, something more 'controlled' like that would be great long-term, but coming up with what exactly that should look like is indeed not so easy :) .
I actually tested this with some poms that didn't have a GroupID and that indeed looked pretty OK.
Jup - it should probably still print it, but for example prefixing it with the "check" it belongs to and perhaps the file it's processing would be really useful to be able to do, and looks like it might be fairly easy to plug into that structure (Famous Last Words). |
another-rex
approved these changes
Mar 5, 2023
There was a problem hiding this comment.
Thanks, looks good to me as a middle ground before #186 is landed.
another-rex
removed this pull request from the merge queue due to the queue being cleared
Does this mean there is action required on my part? |
Ah no, was just trying out the merge queue, turns out still doesn't quite work for our repo. Going to merge this in soon. |
hayleycd
pushed a commit
that referenced
this pull request
Mar 9, 2023
) Co-authored-by: Rex P <[email protected]>
julieqiu
pushed a commit
to julieqiu/osv-scanner
that referenced
this pull request
May 2, 2023
…oogle#253) Co-authored-by: Rex P <[email protected]>
julieqiu
pushed a commit
to julieqiu/osv-scanner
that referenced
this pull request
May 2, 2023
…oogle#253) Co-authored-by: Rex P <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.