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

check: Add function for parsing import names for local packages #139

Merged
merged 1 commit into from Feb 14, 2023

Conversation

jherland
Copy link
Member

This adds find_import_names_from_dep() which will look up a dependency
name in the current Python environment, parse its top_level.txt file (if
it exists), and return the resulting import names made available by this
package.

If the package does not exist in the local environment, or if the
top_level.txt file does not exist for this package, we fall back to the
identity mapping, that is, returning the dependency name itself as an
import name.

Copy link
Collaborator

@Nour-Mws Nour-Mws left a comment

Choose a reason for hiding this comment

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

Wow this was easier than I expected. importlib really does miracles for us!
I think the failing CI is closely related to the issue I was having in #133.

I am only commenting here as I am not sure I understand the motivation behind the choices to let the mapping function fall back to identity mapping.

fawltydeps/check.py Outdated Show resolved Hide resolved
fawltydeps/check.py Outdated Show resolved Hide resolved
tests/test_find_import_names_from_dep.py Outdated Show resolved Hide resolved
@Nour-Mws Nour-Mws added this to the First public release milestone Feb 13, 2023
fawltydeps/check.py Outdated Show resolved Hide resolved
fawltydeps/check.py Outdated Show resolved Hide resolved
@jherland jherland force-pushed the jherland/top_level.txt-parser branch 2 times, most recently from bfe4fdd to 86568cd Compare February 14, 2023 01:02
Copy link
Collaborator

@Nour-Mws Nour-Mws left a comment

Choose a reason for hiding this comment

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

A first nail in the coffin of identity mapping!

(feel free to resolve un-actionable comments or those preceded with Nit: if they don't seem relevant)

fawltydeps/check.py Show resolved Hide resolved
fawltydeps/check.py Show resolved Hide resolved
tests/test_map_dep_name_to_import_names.py Outdated Show resolved Hide resolved
tests/test_map_dep_name_to_import_names.py Outdated Show resolved Hide resolved
tests/test_map_dep_name_to_import_names.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mknorps mknorps left a comment

Choose a reason for hiding this comment

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

Great and clearly presented first stage of incorporating dependency to imports mapping for FD <3

🚀

fawltydeps/check.py Show resolved Hide resolved
This adds find_import_names_from_package_name() which will use
importlib.metadata to look up a package name (e.g. a dependency name)
in the current Python environment, and attempt to find the corresponding
import names that this package provides.

If the package does not exist in the local environment, or if
importlib.metadata is not able to find any provided import names, we
return `None`, and expect the caller to fall back to some other
mechanism to map the package name into import name(s).
@jherland jherland force-pushed the jherland/top_level.txt-parser branch from c0be3eb to bfb7f5f Compare February 14, 2023 13:58
@jherland jherland dismissed vreuter’s stale review February 14, 2023 15:01

Agreed in today's meeting that this PR is good to go.

@jherland jherland merged commit 296975e into main Feb 14, 2023
@jherland jherland deleted the jherland/top_level.txt-parser branch February 14, 2023 15:02
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.

None yet

4 participants