SFTtech / openage

Free (as in freedom) open source clone of the Age of Empires II engine 🚀
http://openage.dev
Other
12.72k stars 1.12k forks source link

More informative `Exception`/Error messages in converter #1344

Open heinezen opened 3 years ago

heinezen commented 3 years ago

Required skills: Python

Difficulty: Medium

In AoE2's .dat format most associations and assignments of properties are done by IDs (e.g. unit has ability with ID X). The openage converter uses these IDs to lookup the associated openage API property and then map the values from AoE2's .dat structure to the corresponding API object's member values. In short, every property from AoE2 needs to be manually mapped to an openage API property. As such, most of the runtime errors are actually lookup errors (usually Python's KeyError) that occur when an AoE2 property was not mapped to an openage API property.

The goal of this task is to make these errors more informative by catching the generic KeyError from Python and re-raising it with a better message. For example, we can improve the error message by specifying:

Example

generic message:

File "openage/convert/processor/conversion/de2/tech_subprocessor.py", line 276, in resource_modify_effect
    upgrade_func = DE2TechSubprocessor.upgrade_resource_funcs[resource_id]
KeyError: 208

better message:

File "openage/convert/processor/conversion/de2/tech_subprocessor.py", line 276, in resource_modify_effect
    upgrade_func = DE2TechSubprocessor.upgrade_resource_funcs[resource_id]
KeyError: No subprocessor function found for handling upgrade of civ resource: 208

Use try-except statement where these lookups could be thrown. Remember to use raise ... from to preserve the stack trace of the initial KeyError.

Further reading:

duanqn commented 3 years ago

When you say

most of the runtime errors are actually lookup errors

Are those errors code defects, or just because the user does not have the correct AoE2 assets?

heinezen commented 3 years ago

Most of them are code defects or missing implementations, e.g.

The correct AoE2 assets should be detected before the conversion starts.

tsherman181 commented 6 months ago

Hi all! I'm a first time contributor and I am going to try to solve this issue. Very new to open source contributing but hoping I can help here!

heinezen commented 6 months ago

@tsherman181 Let us know if you have any problems!

anupriyakkumari commented 2 weeks ago

Hi, could I take this up, please?

heinezen commented 2 weeks ago

@anupriyakkumari Hey yes this is free for implementation :)