Skip to content

Namespaces refactor #5686

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

Merged
merged 8 commits into from
Sep 27, 2018
Merged

Conversation

gvanrossum
Copy link
Member

In preparation of working on PEP 420 namespaces, here's some initial work. IT DOES NOT IMPLEMENT NAMESPACES YET. What it does do:

  • Move nearly 400 lines from build.py to a new file, modulefinder.py. This includes SearchPaths, BuildSource, FindModuleCache, mypy-path, default_lib_path, get_site_packages (now without underscore!), and compute_search_paths.
  • Slight refactor to FindModuleCache so that the search_paths are passed to the constructor instead of to find_module.
  • Removed search_paths and python_executable from the signature of find_module and find_modules_recursive, and also from the cache key (this may be the most controversial change -- it's not just a refactor).
  • Add a (non-functional) --namespace-packages flag to main.py and add new global config option namespace_packages. (These are not used yet.)

I'm presenting this as a separate PR because it's a significant refactor without change of functionality -- everything I plan to do after this will mostly tweak the code in modulefinder.py. It seems fairer to reviewers to be able to review this massive code move without having to worry too much about "what else changed".

(Note that this is part of my general plan to move functionality out of build.py -- that file is (still) way too bulky.)

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

I like this refactoring! build.py is already to large so it is good to move some code out of it.

return dirs

def find_module(self, id: str, search_paths: SearchPaths,
python_executable: Optional[str]) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

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

FWIW python_executable is never actually used by find_module, _find_module, and find_module_recursive, it just gets passed around but I didn't find a place where it is used. As I understand, all the info is stored already in search_paths that are calculated using the executable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's why I removed it from their signatures in the refactor.

self.path = path # File where it's found (e.g. 'xxx/yyy/foo/bar.py')
self.module = module or '__main__' # Module name (e.g. 'foo.bar')
self.text = text # Source code, if initially supplied, else None
self.base_dir = base_dir # Directory where the package is rooted (e.g. 'xxx/yyy')
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra spaces before this comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@gvanrossum gvanrossum merged commit fe6fd1a into python:master Sep 27, 2018
@gvanrossum gvanrossum deleted the namespaces-refactor branch September 27, 2018 17:14
TV4Fun pushed a commit to TV4Fun/mypy that referenced this pull request Oct 4, 2018
In preparation of working on PEP 420 namespaces, here's some initial work. IT DOES NOT IMPLEMENT NAMESPACES YET. What it does do:

- Move nearly 400 lines from build.py to a new file, `modulefinder.py`.  This includes `SearchPaths`, `BuildSource`, `FindModuleCache`, `mypy-path`, `default_lib_path`, `get_site_packages` (now without underscore!), and `compute_search_paths`.
- Slight refactor to `FindModuleCache` so that the `search_paths` are passed to the constructor instead of to `find_module`.
- Removed `search_paths` and `python_executable` from the signature of `find_module` and `find_modules_recursive`, and also from the cache key (**this may be the most controversial change** -- it's not just a refactor).
- Add a (non-functional) `--namespace-packages` flag to `main.py` and add new global config option `namespace_packages`. (These are not used yet.)

I'm presenting this as a separate PR because it's a significant refactor without change of functionality -- everything I plan to do after this will mostly tweak the code in `modulefinder.py`. It seems fairer to reviewers to be able to review this massive code move without having to worry too much about "what else changed".

(Note that this is part of my general plan to move functionality out of `build.py` -- that file is (still) way too bulky.)
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