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

Replaced wild card imports #460

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

GeneCodeSavvy
Copy link
Contributor

@GeneCodeSavvy GeneCodeSavvy commented Feb 28, 2025

Wild card imports were replaced with explicit relative imports for the following files: #433

  1. document.py
  2. location.py
  3. om_compound.py

Additional changes include (based on pylint's suggestion) :

  1. Refactored if else block.
  2. Replaced Dictionary method with Dictionary literal.

EDIT:
Had to replace the import statements in toplevel.py, because build was failing

@GeneCodeSavvy
Copy link
Contributor Author

GeneCodeSavvy commented Mar 1, 2025

Hello everyone, @tcmitchell @jakebeal

I wanted to add that the final build of this PR was failing because of circular import chains, that is, the reason I needed to update toplevel.py.

I am currently working on identified.py and facing the same issue.

  • identified.py imports Document from document.py.
  • document.py imports CustomIdentified from custom.py.
  • custom.py tries to inherit from Identified (from identified.py), creating a circular import chain.

Wildcard Imports Masking Issues:

  • Initially, using from .module import * in both identified.py and custom.py might have inadvertently imported the necessary classes (like Identified) into the right namespace, even with circular dependencies.

Explicit Imports Expose Missing Dependencies:

  • When I switched identified.py to explicit imports, custom.py could no longer access Identified via a wildcard import from identified.py because the class wasn't properly exposed.
  • This led to a NameError because Identified wasn't available in custom.py's scope.
  • Replacing all the wildcard imports with explicit import in all files fixes the problem

I am not sure if you were aware of this. I am assuming yes, and in this case, it is fine to have circular dependencies.
Should I create the next PR with replacing all the wildcard import statements in all the files? It would mean a lot to review in a single PR, but the changes will not be significant. It would also require you to merge this PR to avoid conflicts while I work on other files. This would avoid breaking builds in individual PRs.

Thank you,
Harsh

@tcmitchell
Copy link
Collaborator

I think the wildcard imports are going to be one of the harder things to deal with. Trying to tackle them early is not what I meant when I originally said, "Start with the easy ones and work towards the harder ones."
Your other PRs, each fixing one or two warnings with just a few occurrences are on the right track.
I also think you could eliminate wildcard imports where it is easy to do so, and skip the ones that are circular for now. The goal is not necessarily to make the code completely clean for pylint, but to "Improve pylint compatibility".

@GeneCodeSavvy
Copy link
Contributor Author

Okay, I’ll begin by searching for files that avoid circular imports. Also updating all PR's with respective changes in setup.cfg

I apologize for making you repeat yourself.

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.

2 participants