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

Update doc for import discovery and qualified module names #9949

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

Conversation

akamat10
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ“œ Docs

Description

Refs #9915

Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.80%. Comparing base (4c5393f) to head (e31ad17).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9949   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         174      174           
  Lines       18940    18940           
=======================================
  Hits        18146    18146           
  Misses        794      794           

@akamat10
Copy link
Contributor Author

Don't think changelog needs to be updated for docs based on my understanding.

@jacobtylerwalls jacobtylerwalls added Skip news πŸ”‡ This change does not require a changelog entry Documentation πŸ“— labels Sep 23, 2024
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I feel like this is a great doc after a dive in what exactly the code is doing, but at the same time this so close to being a description of what the code is doing that it's going to be out of sync fast if we ever change the code, that I'm not sure we should add it to the doc. Or maybe with a warning to upgrade the doc in the code ? Waiting for another maintainer opinion about this.

Is another way to do import discovery and work with qualified module name is simpler and make more sense to you @akamat10 ? If so maybe we could change to it directly.

@akamat10
Copy link
Contributor Author

Thank you for going through this. It does require reading through the code :) to know it is correct. Mypy docs for reference where they describe their import discovery. The second option ie default in the doc is very similar to what pylint does if source-roots isn't specified. https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules

Personally I think something like this ought to be in the user guide and leaving it out makes it difficult to understand what is going on. The user may not know where the code is where this is happening. If the logic changes, we should update the doc. I am ok with documenting in the code that the doc should change to reflect it.

Is another way to do import discovery and work with qualified module name is simpler and make more sense to you @akamat10 ? If so maybe we could change to it directly.

I assume you mean changing the current logic, right? I think source-root option comes closest in being clean and being able to control to make it to make it similar to how Python works. I have proposed an alteration in #9955 to simplify it. I would still document after changing the behavior so that the user knows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation πŸ“— Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants