Improve exceptions for abcName()#2014
Conversation
ischoegl
force-pushed
the
improve-exceptions
branch
from
d5853ef to
651a015
Compare
ischoegl
force-pushed
the
improve-exceptions
branch
from
651a015 to
0d94b21
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2014 +/- ##
==========================================
+ Coverage 75.43% 75.47% +0.03%
==========================================
Files 454 454
Lines 56773 56798 +25
Branches 9345 9356 +11
==========================================
+ Hits 42825 42866 +41
+ Misses 10785 10764 -21
- Partials 3163 3168 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ischoegl
force-pushed
the
improve-exceptions
branch
3 times, most recently
from
967a766 to
af53e69
Compare
|
@speth ... this should be fairly straightforward. |
| if (m < m_mm) { | ||
| return m_elementNames[m]; | ||
| } | ||
| throw IndexError("Phase::elementName", "element", m, m_mm); |
There was a problem hiding this comment.
The previous approach using checkElementIndex achieved the same thing, didn't it?
There was a problem hiding this comment.
It is equivalent, but I thought the Phase::elementName labeling is nicer for this specific case. Fwiw, it's parallel to the situation with Phase::speciesName. Technically, the functions only share the if (m < m_mm) { line ... other than the trivial ‘}‘
There was a problem hiding this comment.
Yes, of course the same comment would apply to speciesName. I guess I just think it's odd to change this after you just introduced the return value for checkElementIndex and friends which enabled this when this is one of the few places that function is used in that way. It's a minor point, though, and obviously not something to hold up this PR over.
There was a problem hiding this comment.
Fair point. Was facing an internal tug of war between ‚dry‘ and the most intuitive exception message. At the same time, it’s a really inconsequential choice.
ischoegl
force-pushed
the
improve-exceptions
branch
from
af53e69 to
7da45c4
Compare
ischoegl
force-pushed
the
improve-exceptions
branch
from
7da45c4 to
cf0088c
Compare
2 participants
Changes proposed in this pull request
This is a minor follow-up PR to #2010. I had noticed that exception handling for
*Name(size_t k)was pretty inconsistent. In some instances, an exception was raised, in other cases, an<unknown>was returned, and in other cases, the index wasn't checked at all (i.e., undefined behavior). This PR updates the code so that it consistently throws anIndexErrorif the index is out of bounds.The only instance where transitional behavior is implemented is
kineticsSpeciesName(it's only user-facing, but some user code could check for<unknown>). For 0D and 1D instances ofcomponentName, the implementation was inconsistent (some instances raised exceptions while others didn't), so I opted to raise exceptions for all, so all specializations follow the same approach.Checklist
scons build&scons test) and unit tests address code coverage