DM-53382: Add more descriptive error messages #533
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #533 +/- ##
==========================================
- Coverage 88.94% 88.85% -0.09%
==========================================
Files 150 150
Lines 20727 20760 +33
Branches 2462 2462
==========================================
+ Hits 18435 18446 +11
- Misses 1713 1734 +21
- Partials 579 580 +1 ☔ View full report in Codecov by Sentry. |
timj
left a comment
timj
left a comment
There was a problem hiding this comment.
In theory this is fine but it does make me ponder why some of the APIs being called can't report the information instead.
| overrides.addFileOverride(configPath) | ||
| except Exception as e: | ||
| e.add_note( | ||
| f"Applying config override to task '{label}' from file '{configPath}'" |
There was a problem hiding this comment.
The error message from addFileOverride doesn't include configPath in it?
There was a problem hiding this comment.
I'm not sure about this error specifically. One of the overrides (actually configOverride) was triggered by analysis_tools and took help from Jim to debug because there was no actual reference to analysis_tools anywhere in the traceback. While we added better error messages to that we decided to add them everywhere just in case.
| except Exception as e: | ||
| e.add_note( | ||
| f"Applying config override to task '{label}' for " | ||
| "key '{key}' with value '{value}'" |
There was a problem hiding this comment.
f-string? Maybe addValueOverride should itself report key and value in the error message? Or is that API not raising anything itself but giving a KeyError?
| try: | ||
| config.loadFromStream(buffer, filename=override.ospath, extraLocals=extraLocals) | ||
| except Exception as e: | ||
| e.add_note(f"Applying config override from file '{override.ospath}'") |
There was a problem hiding this comment.
loadFromStream knows the filename but doesn't report it?
| instrument, name = override | ||
| instrument.applyConfigOverrides(name, config) | ||
| try: | ||
| instrument, name = override |
| instrument, name = override | ||
| instrument.applyConfigOverrides(name, config) | ||
| except Exception as e: | ||
| e.add_note(f"Applying config override with instrument '{instrument}' and name '{name}'") |
There was a problem hiding this comment.
All the information in this note seems to be known to applyConfigOverrides.
timj
changed the title
Checklist
package-docs builddoc/changes