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

Consider using specific exceptions rather than generic Exception #15

Closed
xylar opened this issue May 28, 2022 · 7 comments
Closed

Consider using specific exceptions rather than generic Exception #15

xylar opened this issue May 28, 2022 · 7 comments

Comments

@xylar
Copy link
Contributor

xylar commented May 28, 2022

I don't find the exceptions raised by jigsawpy to be very intuitive, and I think that might be true for most python users.

As an example:

raise Exception("Incorrect type: NAME.")

I would expect something like:

    if (not isinstance(name, str)):
        raise TypeError("Argument 'name' to savemsh should be of type str")

As I understand it, the generic Exception class is only intended as a base class for exceptions and shouldn't be used directly.

Python is case sensitive so the use of NAME here also led to some confusion for me.

@dengwirda
Copy link
Owner

Well okay, fair enough --- I can see that one could use the various TypeError, ValueError, etc throughout. Is there a functional reason to prefer this though, is there an issue with try..catch?

NAME is only present in the error string, not in the code, so I'm not sure what the case-sensitive issue you're referring to here is.

@xylar
Copy link
Contributor Author

xylar commented Jun 1, 2022

Well okay, fair enough --- I can see that one could use the various TypeError, ValueError, etc throughout. Is there a functional reason to prefer this though, is there an issue with try..catch?

Yes, one major problem with using Exception as a general practice is that catching Exception is so general that it also catches things like syntax errors in the code block within the try and KeyboardInterrupt, which is what gets raised when you use Ctrl-C.

NAME is only present in the error string, not in the code, so I'm not sure what the case-sensitive issue you're referring to here is.

When I see the error, I search for "NAME" in the code and don't find it. I am used to seeing python errors that use the variable names with the same case that they have in the code.

@dengwirda
Copy link
Owner

Ah, okay --- no problem. Catching things like KeyboardInterrupt seems an unnecessary horror, so I'll add a new "to-do list" issue to swap over to TypeError and friends.

I'm not keen to update some of the other formatting type aspects though --- there's a broader, multi-language style that I try to maintain across jigsaw in general, and some of these quirks are examples of that. For issues of maintainability, as well as my own sanity, many of the comment strings, data structure layouts, etc, etc are as common as possible across jigsaw's underlying C++ implementation as well as the various other language bindings.

@xylar
Copy link
Contributor Author

xylar commented Jun 2, 2022

I'm not keen to update some of the other formatting type aspects though --- there's a broader, multi-language style that I try to maintain across jigsaw in general, and some of these quirks are examples of that. For issues of maintainability, as well as my own sanity, many of the comment strings, data structure layouts, etc, etc are as common as possible across jigsaw's underlying C++ implementation as well as the various other language bindings.

Okay, do as you see fit. I only will reiterate that Incorrect type: NAME. was not a helpful message for me. I had to read the code to figure out what it meant.

@dengwirda
Copy link
Owner

I swapped from Exception to the various TypeError, ValueError and friends in da9e256 which should hopefully clean up the try...catch issues here.

@xylar
Copy link
Contributor Author

xylar commented Jun 8, 2022

I appreciate that.

@dengwirda
Copy link
Owner

Updated to TypeError, ValueError and friends in #17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants