Sitelet https://web.archive.org/web/20220524191354/https://github.com/SFTtech/openage/issues/1344
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More informative KeyError messages in converter #1344

Open
heinezen opened this issue Dec 18, 2020 · 2 comments
Open

More informative KeyError messages in converter #1344

heinezen opened this issue Dec 18, 2020 · 2 comments
Assignees
Labels
assets good first issue improvement python
Projects

Comments

@heinezen
Copy link
Member

@heinezen heinezen commented Dec 18, 2020 •

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:

  • Context of the property (i.e. the property is: ability, resource, unit, tech, ...)
  • Data type that is looked up (function, object, string, other ID, ...)

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:

@heinezen heinezen added improvement python assets good first issue labels Dec 18, 2020
@heinezen heinezen added this to conversion in convert Dec 19, 2020
@duanqn
Copy link
Contributor

@duanqn duanqn commented Dec 23, 2020

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
Copy link
Member Author

@heinezen heinezen commented Dec 23, 2020

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

  • For somehing like tech effects, the converter gets the effect ID and looks up a corresponding handler function in a dict. If the ID is not found, the KeyError should be caught and raise a NotImplementedError.
  • KeyErrors when looking up unit ID or tech IDs are almost always the converters fault
  • Names for converted nyan objects are provided by the converter and a key error here indicates that the name must be added by us

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

@heinezen heinezen self-assigned this Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assets good first issue improvement python
Projects
convert
  
conversion
Development

No branches or pull requests

2 participants