Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files☔ 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
2 participants
Checklist
package-docs builddoc/changes