fix(catalog): resolve Debian/Ubuntu package naming mismatches#39
Conversation
Add candidates field to bat.json (batcat) and fd.json (fdfind) for Debian binary name detection. Add apt method to delta.json with package name git-delta. Implement resolve_apt_package_name() in catalog.py that reads apt package names from catalog available_methods. Wire into upgrade.py so apt-cache policy uses correct Debian package names. Closes #35 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
CybotTM
left a comment
There was a problem hiding this comment.
Review: fix(catalog): resolve Debian/Ubuntu package naming mismatches
What was done well
- The catalog JSON changes (
bat.json,fd.json,delta.json) are all factually correct. Verified against the actual apt repository:batpackage isbat,fdpackage isfd-find,deltapackage isgit-delta. - The
resolve_apt_package_name()function has a clean design with proper fallback chain:available_methods(modern) ->package_managers(legacy) ->packages(legacy) -> tool_name (default). - The
candidatesadditions tobat.jsonandfd.jsoncorrectly address binary name detection on Debian. - The
delta.jsonchanges (addingavailable_methods, changinginstall_methodtoauto) are well-structured and consistent with other catalog entries likebat.json,fd.json, andripgrep.json. - The integration tests in
TestCheckUpgradeAvailableAptResolvedare well-written -- they mocksubprocess.runand verify the exact command-line arguments, which directly validates the fix. - CI passes on Ubuntu and macOS (Windows and lint still pending at time of review).
Issues requiring changes
Important: Two tests are no-ops (2 of claimed 17 tests)
The PR description claims 17 tests, but two of them do not test their stated behavior:
-
test_resolve_apt_package_name_with_legacy_packages_field(tests/test_upgrade.py): The mock setup exits scope before assertions. The final assertionassert result != "php" or result == "php"is a tautology (always True). This test verifies nothing about legacy field handling. -
test_resolve_apt_package_name_with_malformed_json(tests/test_upgrade.py): Same pattern -- mock exits scope, then tests a nonexistent tool (the "file not found" path) instead of the "malformed JSON" path. Thejson.JSONDecodeErrorhandler inresolve_apt_package_name()is never exercised by any test.
These should either be fixed to properly test their stated scenarios or removed. See inline comments for suggested fixes.
Suggestion: Scope clarification
The original issue (#35) identifies three affected code paths: upgrade.py, installer.py, and install_plan.py. This PR only fixes upgrade.py (the apt-cache policy query path). The PR description should note that installer/install_plan fixes are deferred.
Minor observations
- The
except (json.JSONDecodeError, KeyError)inresolve_apt_package_name()includesKeyErrorwhich is unreachable given that all dictionary accesses use.get()with defaults. See inline comment. - The resolver reads catalog JSON from disk on every call rather than reusing the existing
ToolCataloginstance. Not a blocker but worth considering for efficiency in bulk operations. - The
ctagstool (packages.apt: "universal-ctags") andsponge(packages.apt: "moreutils") also have name mismatches but arepackage_managerinstall method tools, so they go through a different code path and are not affected by this specific fix. Good that the scope is contained.
Verdict
The core fix is correct and well-targeted. The two no-op tests need to be fixed before merge -- they give a false sense of coverage for the legacy field and malformed JSON error paths.
- Fix no-op tests for legacy packages field and malformed JSON - Remove unreachable KeyError from except clause in catalog.py Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Summary
candidatesfield tobat.json(batcat) andfd.json(fdfind) for Debian binary detectiondelta.jsonwith package namegit-deltaresolve_apt_package_name()incatalog.pythat reads apt package names from catalogupgrade.pysoapt-cache policyuses correct Debian package namesCloses #35
Test plan
TestCatalogDebianNaming: verifies candidates and apt methods in catalog JSONTestAptPackageNameResolver: covers fd, bat, delta, ripgrep, unknown tools, legacy fields, malformed JSONTestCheckUpgradeAvailableAptResolved: verifiesapt-cache policyreceives resolved package name