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

Resource.get_attributes: incorrect documentation #452

Open
ANogin opened this issue Apr 6, 2024 · 3 comments
Open

Resource.get_attributes: incorrect documentation #452

ANogin opened this issue Apr 6, 2024 · 3 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@ANogin
Copy link
Contributor

ANogin commented Apr 6, 2024

def get_attributes(self, attributes_type: Type[RA]) -> RA:
"""
If this resource has attributes matching the given type, return the value of those
attributes. Otherwise returns `None`.
:param attributes_type:
:return:
"""
attributes = self._resource.get_attributes(attributes_type)
if attributes is None:
raise NotFoundError(
f"Cannot find attributes {attributes_type} for resource {self.get_id().hex()}"
)

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

@whyitfor
Copy link
Contributor

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

@whyitfor whyitfor added documentation Improvements or additions to documentation good first issue Good for newcomers labels Apr 10, 2024
@ANogin
Copy link
Contributor Author

ANogin commented Apr 10, 2024

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
Copy link
Contributor

whyitfor commented Apr 10, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants