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

More informative error message when sasview is not on the python path #510

Merged
merged 3 commits into from
Jul 5, 2022

Conversation

pkienzle
Copy link
Contributor

The import error now says that sasview/src is missing from the python path rather than module sas not found. This is still somewhat cryptic. The full instructions should say that sasview can be downloaded with "git clone https://github.com/sasview/sasview.git" but that is a bit much for an exception text string.

See #467.

Copy link

@wpotrzebowski wpotrzebowski left a comment

Choose a reason for hiding this comment

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

Code looks good. I've also wondered whether this are sufficent instructions for users but I agree we need to keep it brief and add more detailed instructions in sasmodels API (may be there already).

@wpotrzebowski
Copy link

@lucas-wilkins will come up with better suggestion for handling error

@pkienzle
Copy link
Contributor Author

It is not worth any extraordinary efforts. Once the data readers are available as a separate package the issue can be addressed properly.

@lucas-wilkins
Copy link
Contributor

No extraordinary change made

@lucas-wilkins
Copy link
Contributor

No extraordinary change made

or at least there's not now ;)

@pkienzle
Copy link
Contributor Author

Some preferences:

  1. Regarding f-strings, sasmodels still supports python 2.7. If we start using python 3 features then semantic versioning would require that we release it as 2.0. I've been avoiding this by not using python 3 features.

  2. I prefer the "Data loader" not available rather than "sas" not available (which is what ie.name expands to when sasview is not on the path). It feels more user focused. Also, the particular module is available further up on the traceback.

  3. I prefer the same variable name to hold the exception. sasmodels uses exc elsewhere, so use that rather than ie.

Or perhaps change all of them to e since that is the most common in the standard library:

 312 e
 200 exc
  94 err
  90 msg
  17 ex
  15 why
  15 v
  12 pe
   9 error

and in my site-packages directory:

1715 e
 472 err
 394 exc
  77 error
  68 msg
  41 caught_exc
  34 ex
  26 v
  19 exception

SasView uses a mixture of ex in the qt gui, mostly exc in sascalc, and a variety of forms in the data loaders, with e as the most common.

@pkienzle
Copy link
Contributor Author

It looks like I've decided that the matrix multiplication operator @ is compelling enough in another thread. I guess that means f-strings are okay. I'm still not convinced I want to bump to version 2 because of this patch. Maybe 1.1?

@lucas-wilkins
Copy link
Contributor

e, pe, and v are probably equivalent in the sense that they are the lower case initials of the exception class. I would usually use e, or maybe ex. It's just a very unusual thing to catch.

Personally, I would think the overlap between users that know what "Data loader" means, and those that wouldn't prefer a more specific error message, would be small.

Roll it back if you like.

@pkienzle
Copy link
Contributor Author

These are minor preferences. I'll leave it to someone else to decide if they want to rollback or merge as is. Regardless, we should revisit this when the data loaders are available as a separate package.

@lucas-wilkins
Copy link
Contributor

My commits were just a suggestion, I'm happy either way too.

@wpotrzebowski wpotrzebowski merged commit e15a0d2 into master Jul 5, 2022
@wpotrzebowski wpotrzebowski deleted the ticket_467 branch July 5, 2022 13:36
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.

4 participants