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

Ensure that a unique problem module exists before importing #19

Merged
merged 2 commits into from
Jan 7, 2019

Conversation

benzwick
Copy link
Collaborator

This fixes unexpected behaviour that occurs if a problem module exists in more than one location or if a problem module throws an exception during import.

Previously, there were two things that could go wrong:

  1. The problem module is defined in both the examples directory and the current directory in which case the user might be unaware that the example problem takes precedence over the one in the current directory.

  2. If the module exists but raises an exception during import then that exception is caught and an attempt is made to import the module from the current directory instead. Further exceptions are then caught and a RuntimeError is raised which (misleadingly) informs the user that the module could not be found.

This made it difficult to debug example problems because exceptions raised during import were caught by the exception handler.

References:

This fixes unexpected behaviour that occurs if a problem module exists
in more than one location or if a problem module throws an exception
during import.

Previously, there were two things that could go wrong:

1. The problem module is defined in both the examples directory and the
   current directory in which case the user might be unaware that the
   example problem takes precedence over the one in the current directory.

2. If the module exists but raises an exception during import then that
   exception is caught and an attempt is made to import the module from
   the current directory instead. Further exceptions are then caught and
   a RuntimeError is raised which (misleadingly) informs the user that
   the module could not be found.
@benzwick
Copy link
Collaborator Author

Before merging this @mikaem let me just check what happens if a problem module exists somewhere else on Python's path.

The previous commit didn't consider the case where a problem module
is found on Python's path elsewhere than the problems directory or the
current directory.

Instead of checking if multiple problem modules are found; simply print
the origin of the module that is imported.

In the future (Python >= 3.7) we could use ModuleNotFoundError instead.
@benzwick
Copy link
Collaborator Author

The previous commit didn't consider the case where a problem module is found on Python's path elsewhere than the problems directory or the current directory. Now, instead of checking if multiple problem modules are found the origin of the module is printed before it is imported.

@benzwick
Copy link
Collaborator Author

Should I squash this down into a single commit and create a new pull request @mikaem ?

My first attempt at improving the problem module import behaviour was a bit too complicated and doesn't work for all cases. The second one is much simpler but doesn't actually do what is stated in this pull request description. It simply checks if the module exists before attempting to import it. This way exceptions raised during the loading of the module are handled separately to the exception raised if a module is not found.

@mikaem mikaem merged commit 3de552c into mikaem:master Jan 7, 2019
@mikaem
Copy link
Owner

mikaem commented Jan 7, 2019

Thanks for all the fixes @benzwick:-)

@benzwick benzwick deleted the import-problem-module branch January 7, 2019 15:18
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