OpenVoiceOS / ovos_skill_manager

skill installer for OVOS
Apache License 2.0
5 stars 6 forks source link

Uncaught custom exceptions during most operations should warn rather than crash #38

Open ChanceNCounter opened 3 years ago

ChanceNCounter commented 3 years ago

If, for instance, we get one of the GitHub parsing exceptions during osm sync, the malformation of a single skill entry should not crash osm, nor prevent the rest of the sync from finishing.

JarbasAl commented 3 years ago

that should never happen, lets be sure to log it loud and clear if it happens, we need to fix root cause

ChanceNCounter commented 3 years ago

That particular case shouldn't happen anymore, but some of these exceptions exist to break out of routines that would otherwise try to parse a bunch of Nones. It's good that they're raised, it's just bad that raising them crashes osm

ChanceNCounter commented 3 years ago

I now see the confusion. Poorly titled, but I can't think of a better one.

I mean, several existing, uncaught cases of the GitHub-related custom exceptions should warn rather than crashing. I didn't include a list because I discovered the problem in passing. It will require a deeper dive to find the wheres.

NeonDaniel commented 3 years ago

Related to this, I think the get_requirements_json logs should be downgraded from ERROR to WARNING since a skills manifest.yml is not required, and this has a tendency to spam the log when batch installing skills.

NeonDaniel commented 2 years ago

Is this still valid? I thought everything in a loop got a try/except inside already..

JarbasAl commented 2 years ago

the whole point of the dedicated exceptions is throwing/catching them when they make sense

if its uncaught, thats the point of adding it in the first place, go fix it!

imho this issue is wontfix unless i misunderstood

ChanceNCounter commented 2 years ago

You misunderstood. Either this issue is resolved, or we just haven't had enough opportunities to use osm since the GitHub refactor. One way or another, we had a shitload of uncaught exceptions.