fix: normalize package names to lowercase for correct mapping - #552
fix: normalize package names to lowercase for correct mapping#552HeyBuddy-NSK wants to merge 9 commits into
Conversation
PNHD
left a comment
There was a problem hiding this comment.
The one-line change does not address the issues this PR claims to close.
get_pkg_names() only lowercases the mapped/output value. The --use-local path is implemented in get_import_local(), which is unchanged here. In particular, #312 reports a --use-local failure for a capitalized distribution/import name, and #243 reports that --use-local finds imports but writes zero requirements. #476 is a separate duplicate-local-package/venv problem. None of those code paths are changed by this patch.
There is also a mapping-preservation issue with lowercasing the result after lookup: mapped distribution names are data, not lookup keys. The safer direction is to normalize matching at the appropriate comparison/lookup boundary and preserve the mapped PyPI name unless the project intentionally normalizes output globally.
Please add a regression that reproduces at least the specific --use-local case being fixed, change the matching path that fails that regression, and avoid claiming unrelated issues are closed.
Relocates the .lower() normalization from get_pkg_names() to get_import_local(). This preserves the intended case of the mapped output data while correctly resolving the case sensitive dictionary lookup failures triggered by the --use-local flag.
|
@PNHD You were right, while my original oneline change in get_pkg_names() resolved the issue on my end, it was incorrectly altering the final output data rather than just handling the lookup. I traced through get_import_local() and confirmed the match there is case-sensitive (item in package["exports"] or item == package["name"]). I pushed a new commit that normalizes the case directly at that comparison. It now strictly handles the matching logic without modifying the actual output casing. I have updated the description and issues, Let me know if this updated fix looks right to you!. |
PNHD
left a comment
There was a problem hiding this comment.
The lookup-boundary change is now in the right place and fixes the main issue I raised: get_import_local() now does the case-insensitive match without lowercasing the package data that is later written to the requirements output.
I still can't approve this head yet for two concrete reasons:
- The PR description says it adds
test_get_import_local_case_insensitive, but the current8be2a44diff only changespipreqs/pipreqs.pyandpipreqs/mapping; there is no test change. Please add a regression that fails onmasterand passes with this fix, ideally covering case differences in bothpackage["name"]andpackage["exports"]. - #243 is still not demonstrated by this patch. That report finds
pyserial,PySimpleGUI, andPyYAMLand then writes zero local requirements; this head does not include a reproduction showing that case-insensitive matching is the cause. Please either add a regression reproducing #243 and show this change fixes it, or dropCloses #243and keep the PR scoped to the demonstrated #312 case.
Please also remove the unrelated aiofiles:aiofiles mapping from this focused fix, and clean up the commented-out old if plus # print(result_unique) debug artifact.
Once the regression is present and the issue scope matches the evidence, the matching approach itself looks right.
|
@PNHD Thanks for the detailed feedback: Regression test for #312 committed #243 - I installed the original report is from 2021 , so i am dropping the For cleanup, i removed the unrelated |
PNHD
left a comment
There was a problem hiding this comment.
Re-reviewed current head 98f14a884be87527f0f5cd9b6b3d90b1d5978fb7. SECURITY-N/A.
The scope and lookup placement from my previous review are now corrected, and dropping #243 is the right call. Two current-head blockers remain:
- The new regression does not actually isolate the
package["name"]comparison. Its Flask fixture has bothname="Flask"andexports=["Flask"], so that assertion still passes if the name-comparison arm is broken because the exports arm matches. Please make the name-path fixture's exports empty or unrelated, while keeping the separate PyYAML fixture for the exports path, so both branches are independently protected. - The new test also contains definite Flake8 E231 violations (
self,mock_local, dict entries like"name":"Flask", andassertEqual(len(result),1)). This repository'stox.inirunsflake8 pipreqs testswithout ignoring E231. The current GitHub Actions runs areaction_required, so lint has not executed on this head yet.
After those are fixed, the production change itself looks appropriately scoped for #312.
PNHD
left a comment
There was a problem hiding this comment.
Re-reviewed current head 75afc81f14e097011cf735d173ba944a3cbad206. SECURITY-N/A.
Both blockers from my prior review are resolved on this head: the Flask fixture now has an empty exports list, so the package["name"] arm is independently exercised, while the separate PyYAML fixture still isolates the package["exports"] arm; and the E231 spacing violations in the regression were fixed. The PR remains scoped to #312 and preserves the original package/output casing.
The current GitHub Actions are still action_required (approval-gated), not failed. The regression is now structured so each previously case-sensitive branch would fail on master and is satisfied by the case-insensitive comparison in this patch. I found no remaining correctness, test-design, architecture, readability, security, or performance blocker on this exact head. LGTM.
|
@PNHD Thanks for the detailed catch. Test Isolation: the name branch fixture ( Lint: fixed the flake8 E231 violations. Let me know if there is anything else before this is ready to merge. |
|
@PNHD Thanks again for the review!. Looks like the GitHub Actions run needs approval to execute ( |
What this fixes: #312
get_import_local()matched imports against locally installed packages using a case-sensitive comparison, so an import likerequestswouldn't match a locally installedRequestspackage. This is the root cause of #312 (capitalized distribution name causing--use-localfailure).What changed
Normalized both sides of the comparison in
get_import_local()to lowercase. This correctly handles the case-insensitive lookup while entirely preserving the original mapped casing in the output data. Added a regression test (test_get_import_local_case_insensitive) covering both directions of the mismatch.Note on future work
I discovered that
get_locally_installed_packages()relies ontop_level.txtindist-info/egg-info. Packages built with newer PEP-517 backends (like flit or hatchling) do not ship this file, causing them to be missed during local scans. I am opening a separate issue for this architectural gap rather than folding it into this PR.Not included: investigated #243
Found that with current package metadata, none of the three hits a case mismatch, reverting this fix locally produces the same result for them. Not claiming this PR fixes #243; scoping this PR to #312 only.
Closes #312