Skip to content

fix: normalize package names to lowercase for correct mapping - #552

Open
HeyBuddy-NSK wants to merge 9 commits into
bndr:masterfrom
HeyBuddy-NSK:fix/package-name-capitalization
Open

fix: normalize package names to lowercase for correct mapping#552
HeyBuddy-NSK wants to merge 9 commits into
bndr:masterfrom
HeyBuddy-NSK:fix/package-name-capitalization

Conversation

@HeyBuddy-NSK

@HeyBuddy-NSK HeyBuddy-NSK commented Aug 2, 2026

Copy link
Copy Markdown

What this fixes: #312

get_import_local() matched imports against locally installed packages using a case-sensitive comparison, so an import like requests wouldn't match a locally installed Requests package. This is the root cause of #312 (capitalized distribution name causing --use-local failure).

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 on top_level.txt in dist-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

Resolves case-sensitive dictionary lookup failures that prevent capitalized imports (like Requests) from matching their package names in the mapping file. Closes bndr#476, closes bndr#312, closes bndr#243

@PNHD PNHD left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@PNHD PNHD mentioned this pull request Sep 2, 2026
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.
@HeyBuddy-NSK

Copy link
Copy Markdown
Author

@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 PNHD left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The PR description says it adds test_get_import_local_case_insensitive, but the current 8be2a44 diff only changes pipreqs/pipreqs.py and pipreqs/mapping; there is no test change. Please add a regression that fails on master and passes with this fix, ideally covering case differences in both package["name"] and package["exports"].
  2. #243 is still not demonstrated by this patch. That report finds pyserial, PySimpleGUI, and PyYAML and 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 drop Closes #243 and 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.

@HeyBuddy-NSK

HeyBuddy-NSK commented Sep 3, 2026

Copy link
Copy Markdown
Author

@PNHD Thanks for the detailed feedback:

Regression test for #312 committed test_get_import_local_case_insensitive, which mocks the locally installed packages with deliberate case mismatches and verfies get_import_local() now matches correctly while preserving the original casing in the output.

#243 - I installed pyserial, PySimpleGUI, and PyYAML and inspected what get_locally_installed_packages() actually returns for them today. with current package metadata, none of the three hit a case mismatch, their exports already contain an exact-case match to the import name.
I confirmed my fix locally and re running: the outcome is identical either way, this patch doesn't measurably affect these three packages.

the original report is from 2021 , so i am dropping the #243 from this PR and scoping it to #312, which is directly reproduced and tested.

For cleanup, i removed the unrelated aiofiles:aiofiels mapping entry, the leftover commented-out if, and the debug print line.

@HeyBuddy-NSK
HeyBuddy-NSK requested a review from PNHD September 3, 2026 12:23

@PNHD PNHD left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The new regression does not actually isolate the package["name"] comparison. Its Flask fixture has both name="Flask" and exports=["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.
  2. The new test also contains definite Flake8 E231 violations (self,mock_local, dict entries like "name":"Flask", and assertEqual(len(result),1)). This repository's tox.ini runs flake8 pipreqs tests without ignoring E231. The current GitHub Actions runs are action_required, so lint has not executed on this head yet.

After those are fixed, the production change itself looks appropriately scoped for #312.

@PNHD PNHD left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HeyBuddy-NSK

Copy link
Copy Markdown
Author

@PNHD Thanks for the detailed catch.

Test Isolation: the name branch fixture ( Flask ) now has exprots: [], so it can only pass via the package["name"] camparison.

Lint: fixed the flake8 E231 violations.

Let me know if there is anything else before this is ready to merge.

@HeyBuddy-NSK

Copy link
Copy Markdown
Author

@PNHD Thanks again for the review!.

Looks like the GitHub Actions run needs approval to execute (action_required), let me know if you're able to trigger that, or if there's anything else needed on my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The program does not recognize packages with names containing capital letters. --use-local creates empty requirements.txt

2 participants