Skip to content
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

Eliminate Package class #422

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Eliminate Package class #422

merged 2 commits into from
Jul 25, 2024

Conversation

ikretz
Copy link
Contributor

@ikretz ikretz commented Jul 19, 2024

This PR proposes to eliminate the Package class, which superclasses PackageScanner and ProjectScanner.

Reasons to do this:

  1. There is no context in which a PackageScanner and a ProjectScanner can be used interchangeably without a great deal of difficulty. These classes have only one common method, scan_local(), that expect different kinds (as opposed to types) of arguments: a path to a local package directory or archive for PackageScanner versus a path to a requirements.txt file for ProjectScanner

  2. It seems the only purpose of Package is to give scanner.get_scanner() a definite, non-union return type. However, we end up paying elsewhere for this simplicity, namely via the explicit cast in cli._scan() needed only to satisfy the type checker, so it's not clear we gain anything from it. This PR splits get_scanner() into two functions to eliminate the cast

@ikretz ikretz force-pushed the ikretz/fix/package-class branch from a8e6057 to c459557 Compare July 25, 2024 11:02
@ikretz ikretz marked this pull request as ready for review July 25, 2024 11:13
Copy link
Contributor

@christophetd christophetd left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, this is probably legacy from relatively old code.

@ikretz ikretz merged commit 1ab60ec into main Jul 25, 2024
10 checks passed
@ikretz ikretz deleted the ikretz/fix/package-class branch July 25, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants