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

Import start.py using absolute path. #73

Merged
merged 3 commits into from
Oct 6, 2021

Conversation

yakutovicha
Copy link
Member

Previously, the apps folder was considered a python module. Since
this is not true anymore [1,2], the loading of the apps' start.py
needs to be adapted.

[1] aiidateam/aiida-prerequisites#37
[2] aiidalab/aiidalab-docker-stack#213

Previously, the apps folder was considered a python module. Since
this is not true anymore [1,2], the loading of the apps' start.py
needs to be adapted.
[1] aiidateam/aiida-prerequisites#37
[2] aiidalab/aiidalab-docker-stack#213
Copy link
Member

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Looks good. Could you briefly comment about the motivation to use this particular approach as opposed to importlib.machinery.SourceFileLoader? I did some cursory search and found the following article: https://www.geeksforgeeks.org/how-to-import-a-python-module-given-the-full-path/

@yakutovicha
Copy link
Member Author

Looks good. Could you briefly comment about the motivation to use this particular approach as opposed to importlib.machinery.SourceFileLoader? I did some cursory search and found the following article: https://www.geeksforgeeks.org/how-to-import-a-python-module-given-the-full-path/

I didn't find the importlib.machinery.SourceFileLoader approach, before. But since it is shorter, I will use it.

@yakutovicha
Copy link
Member Author

I didn't find the importlib.machinery.SourceFileLoader approach, before. But since it is shorter, I will use it.

The amount of code is the only difference I see between those two approaces. @csadorf are you aware of the other differences?

@csadorf
Copy link
Member

csadorf commented Oct 6, 2021

I didn't find the importlib.machinery.SourceFileLoader approach, before. But since it is shorter, I will use it.

The amount of code is the only difference I see between those two approaces. @csadorf are you aware of the other differences?

No, I am not aware of any differences besides the length and that the currently implemented approach allows one to control the execution of the module. It looks like both approaches are part of the public API and would be equally well-supported.

@yakutovicha
Copy link
Member Author

I didn't find the importlib.machinery.SourceFileLoader approach, before. But since it is shorter, I will use it.

The amount of code is the only difference I see between those two approaces. @csadorf are you aware of the other differences?

No, I am not aware of any differences besides the length and that the currently implemented approach allows one to control the execution of the module. It looks like both approaches are part of the public API and would be equally well-supported.

ok, then the shorter one wins, I think.

@yakutovicha
Copy link
Member Author

changed, tested - works.

Copy link
Member

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Assuming it works, approved! 😃

@yakutovicha yakutovicha merged commit 436ed37 into develop Oct 6, 2021
@yakutovicha yakutovicha deleted the fix/import-start-py-using-absolute-path branch October 6, 2021 13:52
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