Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork910
fix: Proper way of findingnpm packfilename output.#3460
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
asottile commentedMay 9, 2025
the other branch which tried to solve this is here and my recommendation:#3259 (comment) |
ovcharenko commentedMay 9, 2025
That doesn't help much IMHO, because the JSON is part of other noisy output. So instead of one-line fix you will have to find JSON object and handle all error parsing. |
asottile commentedMay 9, 2025
pretty sure in json mode it does a better job of obscuring the nonsense |
ovcharenko commentedMay 9, 2025
19d0b42 should do this, right? |
npm pack output.npm packfilename output.ovcharenko commentedMay 12, 2025
@asottile any chance to get this merged any soon? |
tests/languages/node_test.py Outdated
| "devDependencies": { | ||
| "typescript": "^5.3.0" | ||
| } |
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.
this is going to make the test prohibitively slow -- can you make a faster example?
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.
Uh oh!
There was an error while loading.Please reload this page.
bbabec2 to928edc0Compare| defget_npm_version()->tuple[int, ...]: | ||
| _,out,_=cmd_output('npm','--version') | ||
| version_match=re.match(r'^(\d+)\.(\d+)\.(\d+)',out) | ||
| ifversion_matchisNone: | ||
| return0,0,0 | ||
| else: | ||
| returntuple(map(int,version_match.groups())) |
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.
new code should not end up inutils -- also this is only used in one place
| # https://docs.npmjs.com/cli/v11/using-npm/changelog#1100-pre0-2024-11-26 | ||
| ifnpm_version>= (11,0,0): | ||
| args.append('--ignore-scripts') |
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.
this seems unrelated to your patch / not necessary
| exceptjson.JSONDecodeErrorase: | ||
| raiseValueError('Failed to parse npm pack output as JSON.')frome |
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.
just let it raise no reason to catch and reraise
| ifnotpkg_json: | ||
| raiseValueError('JSON array from npm pack is empty.') | ||
| ifnotisinstance(pkg_json,list): | ||
| raiseValueError('Expected npm pack output to be a JSON array.') | ||
| filename=pkg_json[0].get('filename') | ||
| iffilenameisNone: | ||
| raiseKeyError( | ||
| "Key 'filename' not found in the first element " | ||
| 'of the JSON array.', | ||
| ) |
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 would just let these raise
| def_make_hello_world(tmp_path,package_json=None): | ||
| package_json=package_jsonor'''\ |
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.
rather than changing this function -- call it and then write a package.json in the specific test below
| assertret== (0,b'Hello World\n') | ||
| deftest_node_with_prepare_script(tmp_path): |
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.
can you show the output of this failing before your patch?
That will fix pollution of installation path with TypeScript output: