search

UDST / orca_test

Data assertions for the Orca task orchestrator
BSD 3-Clause "New" or "Revised" License
0 stars 3 forks source link

Overbroad Except Clauses #3

Closed Eh2406 closed 7 years ago

Eh2406 commented 7 years ago

To start with this is my first time looking at the code, so take this as the first impression that it is. I found the except: clause a code smell. It reminds me of this anti-pattern Overbroad Except Clauses Can we be more specific on what we are catching? When we are using it to catch an assert, would an if be clearer?

As always just starting a discussion, and willing to help implement any fix.

smmaurer commented 7 years ago

Yes, this sounds like a good idea!

There are probably cases here and there where try/except is better, like if we're testing whether an Orca object can be generated without errors, but mostly we can get rid of it.

I may have had a similar thought at some point, because it looks like the injectable assertions near the end of orca_test.py mostly use if statements.

If you feel like implementing this, go right ahead!

waddell commented 7 years ago

I'll second that motion. If you have the time to do this and issue a pull request, Jacob, run with it!

Eh2406 commented 7 years ago

Do you have advice for the TODO's?

smmaurer commented 7 years ago

Whoops, i replied to that in the PR, which is now closed.

No, i'm not sure how we should handle the backtraces. One of the things i haven't gotten around to is adding Python 3 cross-compatibility, which might give us better options. (I'll open a new issue for that.)