Yarn info parsing fixes #9328
Merged
Yarn info parsing fixes #9328
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
fviernau
force-pushed
the
yarn-info-parsing-fixes
branch
from
1756004 to
4b81b04
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9328 +/- ##
=========================================
Coverage 67.45% 67.45%
Complexity 1200 1200
=========================================
Files 241 241
Lines 8498 8498
Branches 904 904
=========================================
Hits 5732 5732
Misses 2402 2402
Partials 364 364
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fviernau
force-pushed
the
yarn-info-parsing-fixes
branch
5 times, most recently
from
a12ad89 to
e297b5a
Compare
sschuberth
requested changes
Oct 24, 2024
| internal fun parseYarnInfo(stdout: String, stderr: String): PackageJson? = | ||
| extractDataNodes(stdout, "inspect").firstOrNull()?.let(::parsePackageJson).alsoIfNull { | ||
| extractDataNodes(stderr, "error").forEach { | ||
| logger.warn { "Error parsing yarn info: ${it.jsonPrimitive.content}." } |
There was a problem hiding this comment.
Nits:
- "yarn" -> "Yarn"
- Probably drop the trailing "." as
contentcould already contain it.
There was a problem hiding this comment.
Also, as these are errors on the Yarn side, should be log to error level, too? This would even make more sense when also parsing for warnings explicitly in a follow-up commit, as these would then be logged as warnings.
There was a problem hiding this comment.
As ORT's execution still is "successful" (you can rely on the result), I would propose to log the
errors as "warnings". (And then in a following change log warnings either as info or debug).
Would you agree?
fviernau
force-pushed
the
yarn-info-parsing-fixes
branch
from
e297b5a to
58af405
Compare
fviernau
force-pushed
the
yarn-info-parsing-fixes
branch
from
58af405 to
5f0b70d
Compare
This is a fix-up for ad9a363. Signed-off-by: Frank Viernau <[email protected]>
Signed-off-by: Frank Viernau <[email protected]>
Prepare for an upcoming change that signals that parsing of `PackageJson` data was unsuccessful. Signed-off-by: Frank Viernau <[email protected]>
This function is not suitable for parsing the JSON objects on `stderr` as-is. Signed-off-by: Frank Viernau <[email protected]>
Avoid code duplication in an upcoming change. Signed-off-by: Frank Viernau <[email protected]>
Previously, calling `parseYarnInfo(process.stderr)` was useless, because it attempts to only parse a `PackageJson` instance from `stderr` which always fails, because `yarn` does not output such data to `stderr`. However, in [1] the logging of errors obtained from `stderr` got (partially) removed. Fix this up by logging any errors found in `stderr` in case no `PackageJson` instance could be parsed from `stdout`. [1]: ad9a363 Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
yarn-info-parsing-fixes
branch
from
5f0b70d to
f6f9054
Compare
sschuberth
approved these changes
Oct 24, 2024
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.
See individual commits.
Part of #9261.