Skip to content

fix(import_parser): remove obsolete duplicated local import detection#172

Merged
mkniewallner merged 4 commits intofpgmaas:mainfrom
mkniewallner:fix/171-use-root-directory-to-check-local-imports
Nov 8, 2022
Merged

fix(import_parser): remove obsolete duplicated local import detection#172
mkniewallner merged 4 commits intofpgmaas:mainfrom
mkniewallner:fix/171-use-root-directory-to-check-local-imports

Conversation

@mkniewallner
Copy link
Collaborator

Resolves #171.

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!
  • Documentation in docs is updated

Description of changes

Update _remove_local_file_imports method so that it correctly considers local imports for modules in the root directory of a project.

@codecov-commenter
Copy link

Codecov Report

Merging #172 (a7fbbbc) into main (6f54023) will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #172   +/-   ##
=====================================
  Coverage   95.1%   95.1%           
=====================================
  Files         28      28           
  Lines        895     895           
  Branches     146     146           
=====================================
  Hits         852     852           
  Misses        28      28           
  Partials      15      15           
Impacted Files Coverage Δ
deptry/import_parser.py 96.0% <100.0%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mkniewallner mkniewallner marked this pull request as ready for review November 5, 2022 22:25
@mkniewallner mkniewallner requested a review from fpgmaas November 5, 2022 22:25
@fpgmaas
Copy link
Owner

fpgmaas commented Nov 7, 2022

Thanks for raising the issue. I agree there is indeed room for improvement here.

On the linked pull request: Identifying local modules is preferably done in the ModuleBuilder class, see here. So the import is actually extracted from the file, but it's later flagged as a local import so it will not be included in the identification of dependency issues. So I think it would be good to move the logic you suggested (scanning for .py files in the root directory) also to ModuleBuilder._is_local_directory, and remove _remove_local_file_imports from ImportParser all together.

The reason I initially added this function, was that I encountered the following file structure a lot:

   dir
      main.py
      foo.py

where main.py contains 'from foo import bar', and then the script is run with python dir/main.py. But in hindsight it was not really a good idea to support this.

@mkniewallner mkniewallner marked this pull request as draft November 8, 2022 06:26
@mkniewallner mkniewallner force-pushed the fix/171-use-root-directory-to-check-local-imports branch from 9b7798b to a1daade Compare November 8, 2022 06:45
@mkniewallner mkniewallner force-pushed the fix/171-use-root-directory-to-check-local-imports branch from 2b25b14 to fb225f0 Compare November 8, 2022 07:45
@mkniewallner mkniewallner marked this pull request as ready for review November 8, 2022 07:55
@mkniewallner
Copy link
Collaborator Author

Thanks for raising the issue. I agree there is indeed room for improvement here.

On the linked pull request: Identifying local modules is preferably done in the ModuleBuilder class, see here. So the import is actually extracted from the file, but it's later flagged as a local import so it will not be included in the identification of dependency issues. So I think it would be good to move the logic you suggested (scanning for .py files in the root directory) also to ModuleBuilder._is_local_directory, and remove _remove_local_file_imports from ImportParser all together.

The reason I initially added this function, was that I encountered the following file structure a lot:

   dir
      main.py
      foo.py

where main.py contains 'from foo import bar', and then the script is run with python dir/main.py. But in hindsight it was not really a good idea to support this.

Thanks for the heads up, I've updated the PR to remove the method altogether, but the logic implemented in the first commit doesn't really apply to the logic in ModuleBuilder, so I didn't update anything in there, besides moving the logic outside the class for performance reasons (so that we only build the list of local modules once instead of each time we build a module).

I think that the current logic to determine if a module is local is a sane one, we just need to make it a bit more configurable to handle cases where projects use src directories to hold the library code (which is quite common nowadays). Right now, on projects using an src directory, the module inside will not be recognised as an internal one.

@mkniewallner mkniewallner changed the title fix(import_parser): use root directory to detect local imports fix(import_parser): remove obsolete duplicated local import detection Nov 8, 2022
@mkniewallner mkniewallner merged commit 548d16a into fpgmaas:main Nov 8, 2022
@mkniewallner mkniewallner deleted the fix/171-use-root-directory-to-check-local-imports branch November 9, 2022 10:04
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.

Some imports are wrongly considered as local ones

3 participants