Skip to content

Use direct import if possible, otherwise use legacy import.#112

Merged
da-h merged 2 commits intoda-h:masterfrom
sbrodehl:dev-direct-import
Sep 14, 2022
Merged

Use direct import if possible, otherwise use legacy import.#112
da-h merged 2 commits intoda-h:masterfrom
sbrodehl:dev-direct-import

Conversation

@sbrodehl
Copy link
Copy Markdown
Collaborator

This PR tries to import modules directly with the given module name/path if possible (assuming the module is a regular python module in the project).

Otherwise, the legacy code is run.

Description

Problem:
Previously, serializing code, where miniflask modules are used, resulted in errors, since the import paths and names of the modules are changed (randomly).

New Behavior:
Now, if mf modules are actual python modules, a direct import is used.

Check all before creating this PR:

  • Documentation adapted
  • unit tests adapted / created

@sbrodehl sbrodehl requested a review from da-h August 12, 2022 19:55
@da-h
Copy link
Copy Markdown
Owner

da-h commented Aug 14, 2022

Hey there,

thanks for tackling this. I am sure that removing the rather ugly random package ids would improve the user experience in the general used case.

I am a bit puzzled by the missing github actions / unit tests for this PR. Do you see what's going on?

@sbrodehl
Copy link
Copy Markdown
Collaborator Author

Hey @da-h,

I don’t know why they don’t show up here, but the actions are run at the forks location https://github.com/sbrodehl/miniflask/actions

@da-h
Copy link
Copy Markdown
Owner

da-h commented Sep 12, 2022

Hey there,

sorry for the late response. ;)

I very much like this approach, however, the tests are currently failing.
Is possibly the parent-folder / module directory missing in the importname?

Best,
da-h

@sbrodehl
Copy link
Copy Markdown
Collaborator Author

Hey @da-h ,

I'm happy to fix any upcoming issues, can you specify the failing tests?
As far as I am aware, everything looks fine in this action run, which is based on the latest commit in this PR (and yes the first one has failing tests).

Anyway, happy to do more work here, if required!

@da-h
Copy link
Copy Markdown
Owner

da-h commented Sep 14, 2022

You are absolutely right. i was mistaken about the message in the line and only saw the failed tests from the commit before it.

Sorry for the confusion and thanks a lot for the contribution!

Best,
da-h

@da-h da-h merged commit 55b350b into da-h:master Sep 14, 2022
github-actions Bot pushed a commit that referenced this pull request Sep 14, 2022
Use direct import if possible, otherwise use legacy import.
@sbrodehl sbrodehl deleted the dev-direct-import branch September 14, 2022 22:32
da-h added a commit that referenced this pull request Oct 5, 2022
da-h added a commit that referenced this pull request Oct 5, 2022
@da-h da-h mentioned this pull request Oct 5, 2022
2 tasks
da-h added a commit that referenced this pull request Oct 6, 2022
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