Make _error_check throw exception for all errcodes

[dmcdougall]

Closes #172

Instead of swallowing the exception and turning into a warning. Fixes
issue #172.

[bemcdonnell]

@dmcdougall, Thanks for the contribution! Looks like a couple tests need to be updated as well

[dmcdougall]

Cool, will get to those shortly. Thanks for the feedback.

[bemcdonnell]

@dmcdougall, Do you plan to update the tests? If not, no problem - I can see if someone else can support you

[michaeltryby]

There is new api that fixes this problem in the swmm-python package.

[michaeltryby]

pyswmm could be the first package to build off the swmm-python foundation.

[dmcdougall]

I’m happy to update the tests.

Sorry for the slow response. I can do this tomorrow.

Michael’s suggestion points to a different way to handle this situation. Which direction do you folks prefer? I can update the tests to swallow an expected exception (no simulation running) which seems reasonable for a test suite. Or I can wait to build off of swmm-python.

[bemcdonnell]

We are working toward moving this project away from ctypes to use the swig wrapped swmm-python but it will be quite a bit of work to flush out the swmm-python. Could be looking at many weeks/months actually. I think knowing this, updating tests would be nice; but it’s not on par with the long term objectives. In any event updating pyswmm to use swmm-python is going to be a fairly large effort. Swmm-python, today, only wraps the legacy api of swmm which is ~7 functions. The OWA swmm API has ~40 functions. When we make the next release of pyswmm using swmm-python as its low level interface to swmm, we will actually bump the release number to 1.0.0.

I think the options are

1. Update the tests and we can make a quick release
2. Keep everything as is and we can close this PR and this issue will take care of itself when we migrate to swmm-python.

[michaeltryby]

Sure effort is involved but I disagree with your characterization that it’s going to be a “large” one. There is one technical hurdle remaining, how to handle structs. Then it’s smooth sailing.

[dmcdougall]

What would you like me to do? Fix up the PR or close it?

If I fix up the PR, the user will see graceful failures instead of seeing warning. A consequence of this is that because of a current bug in swmm (pyswmm/Stormwater-Management-Model#193) their application won’t exit with a useful message.

If I don’t fix up the PR, the user is left with a warning instead of an exception and could be misled into believing swmm is happy with an invalid internal state.

I can see arguments on both sides.

[bemcdonnell]

@dmcdougall, If you're keen to fix it (an update the tests), I'd say go for it! We will migrate the backend of this tool to use swmm-python ASAP. We can release a patch fix after you make this change

[dmcdougall]

Ok, then I’ll update the tests. Thanks for the guidance.

[dmcdougall]

Hmmm, all the tests pass locally for me. I think I have to do a little more digging...

[dmcdougall]

Ah, right. The error code is 309 which indicates a problem writing an output file. I suppose the filesystem footprint on AppVeyor is limited and that's perhaps why. I'll have a think about how to handle those.

[dmcdougall]

I haven't forgotten about this.

I think the best thing to do with the tests is to check the error code in the except block and if it isn't code 309 then use six's reraise capability.

[bemcdonnell]

@dmcdougall, i'm going to close and re-open to see if the Scrutinizer and Circle CI integrations go away... not sure what's going on with them...

[bemcdonnell]

@dmcdougall I made a couple changes and got the tests to pass again in CI. We will be revisiting the error management down the road. In any event- Thanks for your help!