redballoonsecurity / ofrak

OFRAK: unpack, modify, and repack binaries.
https://ofrak.com
Other
1.82k stars 127 forks source link

`Resource.get_attributes`: incorrect documentation #452

Open ANogin opened 2 months ago

ANogin commented 2 months ago

https://github.com/redballoonsecurity/ofrak/blob/01996cf72ad48360444599a236c49f2022784fd7/ofrak_core/ofrak/resource.py#L948-L959

Documentation specifies returning None when resource is not found, but the actual implementation is to raise an exception.

whyitfor commented 2 months ago

@ANogin good catch. Are you planning on making a PR that fixes this?

ANogin commented 2 months ago

Not even sure whether the right thing is to fix the documentation or the code (I would guess documentation because the code change is likely to be very disruptive, and presumably there are plenty of cases where the caller knows the result is not None and having to every tell mypy that is annoying - of course there are also cases where the unexpected unhandled exception could things to break in harder to understand ways).

P.S. BTW, only after submitting the issue I realized that the code listing's "reference in new issue" context menu for a line bypasses the usual issue template... Not sure whether it is possible+desired to create a separate template for that.

whyitfor commented 2 months ago

@ANogin, yes, I'd recommend that you:

  1. Update the dostring so that it accurately reflects the function signature, including noting what it :raises: -- see https://ofrak.com/docs/contributor-guide/getting-started.html#functions-and-methods.
  2. As a sanity check, look at usage of Resource.get_attributes in the ofrak repository. If any usage assumes it can return None, we will want to update them (I think this is mainly for clarity for the future)
    1. Example of correct usage: https://github.com/redballoonsecurity/ofrak/blob/master/ofrak_core/ofrak/core/flash.py#L303
    2. Example of incorrect usage: https://github.com/redballoonsecurity/ofrak/blob/master/ofrak_core/test_ofrak/unit/resource_view/test_view.py#L117