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

Correctly detect transitive dependencies with different module names #1033

Merged
merged 9 commits into from
Jan 25, 2025

Conversation

Willa1228
Copy link
Contributor

I added another check method to verify whether a package exists in the environment, if can't be found by _get_package_name_from_metadata. Also updated the test cases to include bs4 for testing in test_cli_requirement_in.

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added or modified a feature, documentation in docs is updated

Description of changes

Hello, I made a small change to fix the issue where some packages might not have the same name when import, which caused them to be incorrectly reported as missing instead of transitive, as described in in #827.
To ensure the correct package name is retrieved in module.py, I added an additional approach to check if the package is in the development environment:

`

def _get_package_name_from_metadata(self) -> str | None:
    """
    Most packages simply have a field called "Name" in their metadata. This method extracts that field.
    """
    try:
        name: str = metadata(self.name)["Name"]
    except PackageNotFoundError:
        name = self.is_package_installed(self.name)
        if name:
            return name
        return None
    else:
        return name

def is_package_installed(self, package_name: str):
    if importlib.util.find_spec(package_name):
        return package_name
    return None

`


This ensures that the package name can always be retrieved if it is in the environment, regardless of whether the package name and import name match. I also updated the test in test_module so that the package name for bs4 can now be detected.
I'm not sure if this adheres to your program framework or design logic, but I hope it helps. Thanks!
Description of changes

Willa1228 and others added 5 commits January 9, 2025 20:32
…ongl

Added another check method to verify whether a package exists in the environment, if can't be found by _get_package_name_from_metadata.
Also updated the test cases to include bs4 for testing in test_cli_requirement_in.
…ments.in

I revised module.py as per the change requests and added beautifulsoup4==4.12.3 to requirements.in. With this change, the test_cli_single_requirements_files test now passes without any DEP003 issues related to bs4. Additionally, since soupsieve is added to requirements.txt during the uvx --from pip-tools pip-compile process as a dependency of bs4, this causes a DEP002 issue in test_cli_multiple_requirements_files. Therefore, I also revised these two parts of the test case.
…equest

Add a unit test to test the new function _is_package_installed in module.py, also revise the test case in project_requirements_in to verify the use of the new function.
Comment on lines 7 to 8
--index-url https://pypi.nuwainfo.com/nuwa/dev/+simple/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--index-url https://pypi.nuwainfo.com/nuwa/dev/+simple/

tests/unit/test_module.py Outdated Show resolved Hide resolved
tests/unit/test_module.py Outdated Show resolved Hide resolved
tests/unit/test_module.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.1%. Comparing base (a5d01d8) to head (449f8f5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1033   +/-   ##
=====================================
  Coverage   93.0%   93.1%           
=====================================
  Files         37      37           
  Lines        994    1000    +6     
  Branches      99      99           
=====================================
+ Hits         925     931    +6     
  Misses        55      55           
  Partials      14      14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mkniewallner mkniewallner left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

Could you merge main into your branch, as we require branches to be up-to-date before merging (or allowing changes from maintainers so that I can do it, alternatively)?

@mkniewallner mkniewallner changed the title Related to #1009 Correctly detect transitive dependencies with different module names Jan 23, 2025
@Willa1228
Copy link
Contributor Author

Done, so sorry I missed that part

@mkniewallner
Copy link
Collaborator

Branch is again outdated. Can you please enable edit from maintainers so that we avoid both losing more time on this?

@Willa1228
Copy link
Contributor Author

Branch is again outdated. Can you please enable edit from maintainers so that we avoid both losing more time on this?

Since there's no edit from maintainers button (I guess that's because the fork is on a team repository, but I don't have the authority to manage access authority), hence I fork a same repository on my own account and I already add you into it. Hope this works, so sorry for the inconvenience and really thank you for your help.

@Willa1228
Copy link
Contributor Author

Hi again, I just added you into the https://github.com/nuwainfo/deptry/tree/nuwainfo/main (the original fork) so you can edit on both the repository(the nuwainfo or the one on my account). And sorry again for the inconvenience, I am really not good at using GitHub, and I truly appreciate your patience.

@mkniewallner
Copy link
Collaborator

Branch is again outdated. Can you please enable edit from maintainers so that we avoid both losing more time on this?

Since there's no edit from maintainers button (I guess that's because the fork is on a team repository, but I don't have the authority to manage access authority), hence I fork a same repository on my own account and I already add you into it. Hope this works, so sorry for the inconvenience and really thank you for your help.

Ha, did not know about this limitation! If you merge main into your branch one last time we should be good, there is no other pending PR to be merged.

@mkniewallner mkniewallner merged commit ddbb004 into fpgmaas:main Jan 25, 2025
22 checks passed
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.

Imports with package names different from module names are wrongly reported as missing instead of transitive
2 participants