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

TODO, by_s_m.38 - error description? #14

Open
DougCollinge opened this issue May 18, 2017 · 7 comments
Open

TODO, by_s_m.38 - error description? #14

DougCollinge opened this issue May 18, 2017 · 7 comments
Assignees

Comments

@DougCollinge
Copy link
Collaborator

The TODO suggests replacing an error signal return of -99 with a crash but I'm not sure that this is the right thing to do. I don't like the way it works now because -99 has no intrinsic meaning, it just shows up on the report and you the human reader are just supposed to intuit that "something went wrong" and ignore the rest of the report. That is seriously bogus, IMHO. What I think should actually happen here is that the report should just be replaced with a detailed explanation of why the report could not be produced. In order to accomplish this by_s_m needs to be modified to allow an exit that conveys the error message, whatever it is. Perhaps it should always return a list containing an error code, error explanation, and data, if no errors.

@DougCollinge
Copy link
Collaborator Author

In fact, by_s_m has another error situation not handled at line 44. This problem should be rolled into the same fix, IMHO.

@DougCollinge
Copy link
Collaborator Author

by_s_m3 has the same issues, of course.

@boshek
Copy link
Owner

boshek commented May 22, 2017

Absolutely. -99 isn't helpful at all. At least something that gives the user an idea.

@DougCollinge DougCollinge self-assigned this May 22, 2017
@DougCollinge
Copy link
Collaborator Author

DougCollinge commented May 22, 2017

It seems that there is a package that implements try/throw/catch exceptions, which would do this job in a standard way. I'm reluctant to add a heavy dependency, however, just for this little problem. Is there something like that in that "master" roll-up package we already depend on? Sorry, can't remember what it is called.

... Nevermind. I see that tryCatch() is in the base so I'll look into using that. I think it is really better practice to use the exceptions features of the host language to handle exceptions, as opposed to cooking up yer very own scheme, as I proposed above.

@DougCollinge
Copy link
Collaborator Author

DougCollinge commented May 22, 2017

Ok, tentatively done for by_s_m and by_s_m3. You just use stop() to throw an error and tryCatch can catch the error. I don't know how to document this possibility so I haven't done it. We need a test for both these functions but Sam commented out the test file because testing against FORTRAN output no longer works. So we should still test them with some fake data. I can also test the error if I have some data.

@DougCollinge
Copy link
Collaborator Author

This new code passes the tests as-is but the various things that call by_s_m should maybe catch the stop() and do something about it. It isn't a danger because the error only arises if someone bungles the input parameters, so a simple hard stop() so they can fix their mistake is probably appropriate.

@boshek
Copy link
Owner

boshek commented May 24, 2017

A stop and then maybe an informative error message?

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

No branches or pull requests

2 participants