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

Improve error message when loading omnistat modules #141

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jordap
Copy link
Collaborator

@jordap jordap commented Jan 8, 2025

The check before loading omnistat modules with the try/except block is no longer necessary, and generally leads to confusing error messages.

For example, if one of the dependencies is missing, the current approach will only show the error message saying the omnistat module couldn't be loaded and asking to verify installation. After removing this check, Python will just fail to import the missing module, making it easier to understand what's going on.

@jordap jordap marked this pull request as ready for review January 8, 2025 21:04
@jordap jordap requested a review from koomie January 8, 2025 21:04
@koomie
Copy link
Collaborator

koomie commented Jan 9, 2025

The problem is that for some folks, the resulting error message from an import failure will be even more confusing and 99% of the time we know that the failure is due to a lack of pre-installing the dependencies. If you want to adjust this, I would prefer that we keep the try/catch approach, but amend to catch the exception error string and show that after the current error (or expanded current error that suggest to double check dependencies have been installed).

@jordap
Copy link
Collaborator Author

jordap commented Jan 9, 2025

OK, I see what you mean. I find the current message really confusing, and I'd rather see the standard Python error.
But I understand that may not be ideal for everyone.
I'll try to change it and show both.

@koomie
Copy link
Collaborator

koomie commented Jan 9, 2025

OK, I see what you mean. I find the current message really confusing, and I'd rather see the standard Python error. But I understand that may not be ideal for everyone. I'll try to change it and show both.

Thanks, I think that will be good compromise for basic users and developer types...

@jordap jordap force-pushed the jorda/skip-load-check branch from 3997439 to 6e73a58 Compare January 10, 2025 00:27
@jordap
Copy link
Collaborator Author

jordap commented Jan 10, 2025

I changed it so it shows the following message:

Unable to load Omnistat: No module named 'examplemodule'
Please verify installation and dependencies.

This way it shows the exact module that Python is failing to find, and we still keep the nicer error message.

@jordap jordap changed the title Skip check before loading omnistat modules Improve error message when loading omnistat modules Jan 10, 2025
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