-
-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: properly report errors when lock file cannot be parsed #2020
base: main
Are you sure you want to change the base?
Conversation
The function returns only 4 arguments upon errors, but 5 are expected and returned when everything succeeded.
7d5af4b
to
8d2043a
Compare
TestFailed tests (1)//npm/private/test:test_parse_pnpm_lock_test_0 [k8-fastbuild] 40ms 💡 To reproduce the test failures, run
Teste2e/bzlmodAll tests were cache hits 5 tests (100.0%) were fully cached saving 857ms. Teste2e/gyp_no_install_scriptAll tests were cache hits 2 tests (100.0%) were fully cached saving 503ms. Teste2e/js_image_ociAll tests were cache hits 1 test (100.0%) was fully cached saving 7s. Teste2e/npm_link_packageAll tests were cache hits 3 tests (100.0%) were fully cached saving 820ms. Teste2e/npm_link_package-esmAll tests were cache hits 3 tests (100.0%) were fully cached saving 1s. Teste2e/npm_translate_lockAll tests were cache hits 1 test (100.0%) was fully cached saving 34ms. Teste2e/npm_translate_lock_emptyAll tests were cache hits 1 test (100.0%) was fully cached saving 34ms. Teste2e/npm_translate_lock_multiAll tests were cache hits 2 tests (100.0%) were fully cached saving 171ms. Teste2e/npm_translate_lock_partial_cloneAll tests were cache hits 1 test (100.0%) was fully cached saving 129ms. Teste2e/npm_translate_lock_replace_packagesAll tests were cache hits 3 tests (100.0%) were fully cached saving 503ms. Teste2e/npm_translate_lock_subdir_patchAll tests were cache hits 1 test (100.0%) was fully cached saving 222ms. Teste2e/npm_translate_package_lockAll tests were cache hits 1 test (100.0%) was fully cached saving 131ms. Teste2e/npm_translate_yarn_lockAll tests were cache hits 1 test (100.0%) was fully cached saving 131ms. Teste2e/package_json_moduleAll tests were cache hits 1 test (100.0%) was fully cached saving 590ms. Teste2e/pnpm_lockfilesAll tests were cache hits 40 tests (100.0%) were fully cached saving 3s. Teste2e/pnpm_workspaceAll tests were cache hits 10 tests (100.0%) were fully cached saving 3s. Teste2e/pnpm_workspace_rerootedAll tests were cache hits 12 tests (100.0%) were fully cached saving 2s. Teste2e/repo_mappingAll tests were cache hits 2 tests (100.0%) were fully cached saving 474ms. Teste2e/rules_fooAll tests were cache hits 2 tests (100.0%) were fully cached saving 470ms. Teste2e/runfilesAll tests were cache hits 1 test (100.0%) was fully cached saving 443ms. Teste2e/vendored_nodeAll tests were cache hits 1 test (100.0%) was fully cached saving 199ms. BuildifierFormatFormatting check has failed 💡 Some formatting failures can be fixed automatically by running the command below, while others may require manual fixes
|
@@ -531,12 +531,12 @@ def _parse_lockfile(parsed, err): | |||
A tuple of (importers dict, packages dict, patched_dependencies dict, error string) | |||
""" | |||
if err != None or parsed == None or parsed == {}: | |||
return {}, {}, {}, err | |||
return {}, {}, {}, -1, err if err != None else "could not parse Yaml config" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, you now return a 5-tuple instead of a 4-tuple, doesn't that require changes at the callsites?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call sites already expect 5-tuple 😄 — they never see the invalid 4-tuple though because of no errors most of the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, do you mind correcting the Returns
doc above in that case?
The function returns only 4 arguments upon errors, but 5 are expected and returned when everything succeeded.