Skip to content

Remove filesystem imports#120

Merged
da-h merged 15 commits intov5-devfrom
remove_filesystem_imports
Dec 20, 2022
Merged

Remove filesystem imports#120
da-h merged 15 commits intov5-devfrom
remove_filesystem_imports

Conversation

@da-h
Copy link
Copy Markdown
Owner

@da-h da-h commented Oct 7, 2022

Remove Filesystem Imports

Attention: This is a breaking change! (Search below for the term API-CHange).

This MR changes the way miniflask imports modules.
Previously, miniflask would overwrite the python import-routine to allow module repositories in the whole filesystem.
This however, leads to a variety of problems, for instance harder to track errors in testing environments, instantiations of objects for relative imports compared to instantiations given through miniflask-routines, etc.

The solution to all this is to let miniflask determine the import paths that python would use for all module imports and use those for importing.
With this in place, renaming module-base directories internally, or, allowing filesystem-wide imports is not possible anymore.

Nevertheless I strongly believe that making miniflask mimic python's behavior for module naming is more natural and surely more future-proof with regards to understanding the internal behavior as a user and also for debugging for later developments.

*Note: Merging this PR closes #115 and closes #97.

Example

Consider the following directory-structure:

myproject (main project directory)
├── main.py
├── __init__.py
├── subpackage
│   ├── __init__.py
│   ├── usingminiflask.py (first file that uses miniflask)
│   ├── modules
│   │   ├── __init__.py
│   │   ├── module1/ (contents omitted)
│   │   ├── otherdir
│   │   │   ├── __init__.py
│   │   │   ├── module2/ (contents omitted)
....
  • Note that usingminiflask.py sits itself in a python package named subpackage. This is the actual file that calls mf.init in a way.
  • Considering usingminiflask.py, the accessible modules are called modules.module1 and modules.otherdir.module2.
  • Previously, the have been included by overwriting the python-internal importlib-spec objects as the packages modules.module1 and modules.otherdir.module2.
  • With this PR in place, their internal importnames are myproject.subpackage.modules.module1 and myproject.subpackage.modules.otherdir.module2, as myproject.subpackage is the actual python-prefix for all modules.
  • I am free to discuss whether these prefixes (myproject.subpackage) should be also added to the miniflask-ids, however, the current design decision is not to do that. In theory, such a change should be doable without breaking to much, however I am not yet sure to be honest. In theory some users might be depending on absolute paths to work. If for instance those do not rely on state.all, everything should be fine, though.

Things done in this MR

The core commit for this MR is probably 83f69641, where the true python import name of each module specification is determined and used to import a module.

There are other things that needed to be done to make this a complete function. Here in more detail:

  • API-Change: As filesystem-wide imports and renamings are not possible anymore, this MR removes the possibility to give mf.init a dict for initialization.
  • API-Change: We have allowed module names with a .. This is not possible anymore due to the requirement of being also python-import-compatible.
  • Refactored the module_spec["importname"] to be exactly the same as python's import-name for the respective modules, also for miniflask's pre-shipped modules.
  • Fixed a minor bug, where base-directory-modules would end with a ..
  • To make the tests work with pytest on every level, all tests needs to be a python-packages themselves.
    I used the opportunity to normalize the folder-structure a bit.
  • Added some tests to show how to choose only some modules from a module repository.

Check all before creating this PR:

  • Documentation adapted
  • unit tests adapted / created

@da-h da-h requested a review from sbrodehl October 7, 2022 13:31
@da-h da-h force-pushed the remove_filesystem_imports branch 4 times, most recently from d0b1815 to b98047d Compare October 7, 2022 13:39
@da-h
Copy link
Copy Markdown
Owner Author

da-h commented Oct 7, 2022

Pushes fix linter errors.
(For future da-h: Never think, it'll be ok).

@sbrodehl
Copy link
Copy Markdown
Collaborator

For my comprehension: Does removing the filesystem imports completely removes the use of the deprecated load_module functionality?

Comment thread src/miniflask/__init__.py
Comment thread src/miniflask/miniflask.py Outdated
Comment thread src/miniflask/miniflask.py
Comment thread src/miniflask/util.py Outdated
Comment thread src/miniflask/util.py
@da-h da-h force-pushed the remove_filesystem_imports branch from b98047d to b8164c7 Compare October 12, 2022 12:13
Copy link
Copy Markdown
Collaborator

@sbrodehl sbrodehl left a comment

Choose a reason for hiding this comment

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

Let's implement "inclusive" traversal using sys.path and pkgutil.

@da-h
Copy link
Copy Markdown
Owner Author

da-h commented Oct 14, 2022

Hey there, I have a slightly new take on this based on our discussion.

However, as it still feels to be hacky due to the requirement of getting the absolute identifier of the callees package, I have pushed this to a seperate branch, https://github.com/da-h/miniflask/tree/remove_filesystem_imports_new .
This solution however, while allowing system-wide modules from sys.path, it still requires the "hacky" trick to determine the files own package name as seen from python.

Removing the hacky part i.e. removing the relative import capability of modules is also possible, though it induces problems in all test scenarios.
Reason is that pytest adds the respective paths automatically to sys.path, effectively disabling a similar directory structure in different test-cases. To solve this without the "hack" one would have to add the package-prefix to the respetive tests to all test cases which would however disable the possibility to start just a subset of the tests using pytest.
I have also added a branch for this, if you would like to look into this possibility, https://github.com/da-h/miniflask/tree/remove_filesystem_imports_new2 .

Feel free to play around here. :)
But keep in mind, that pytest adds the respective base-directories to sys.path, effectively requiring to let some code "know" its package-identifier.

@da-h da-h force-pushed the remove_filesystem_imports branch from b8164c7 to 843ba7c Compare December 19, 2022 16:07
@da-h
Copy link
Copy Markdown
Owner Author

da-h commented Dec 19, 2022

Rebased onto v5-dev + replaced with the branch remove_filesystem_imports_new mentioned above.

@sbrodehl sbrodehl self-requested a review December 19, 2022 16:12
Copy link
Copy Markdown
Collaborator

@sbrodehl sbrodehl left a comment

Choose a reason for hiding this comment

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

Please address the linter issues.

@da-h da-h force-pushed the remove_filesystem_imports branch from 843ba7c to 09ea11a Compare December 19, 2022 16:18
@da-h da-h requested a review from sbrodehl December 19, 2022 16:21
@da-h da-h force-pushed the remove_filesystem_imports branch from 3452bcf to 6cfeac4 Compare December 19, 2022 17:29
@da-h da-h force-pushed the remove_filesystem_imports branch from 6cfeac4 to 0ae1033 Compare December 19, 2022 17:31
@da-h da-h requested review from sbrodehl and removed request for sbrodehl December 19, 2022 17:33
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.

Remove filesystem imports DeprecationWarning: the load_module() method is deprecated.

2 participants