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

Add TargetFile.custom() documentation when building readthedocs #1749

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Dec 23, 2021

Fixes #1602

Description of the changes being introduced by the pull request:

Add the missing Targetfile.custom() property to the RTD documentation.
This property is not documented and is not visible in RTD documentation:
I think it would be useful to expose as the "supported" way for a client app to read custom metadata for a target.

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@coveralls
Copy link

coveralls commented Dec 23, 2021

Pull Request Test Coverage Report for Build 1663921369

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.737%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tuf/api/metadata.py 0 1 0.0%
Totals Coverage Status
Change from base Build 1658257798: 0.0%
Covered Lines: 4092
Relevant Lines: 4171

💛 - Coveralls

@MVrachev
Copy link
Collaborator Author

MVrachev commented Dec 23, 2021

I managed to add the TargetFile.custom() property in the RTD documentation.
Now I want to research how can I add information about the custom type: Dict[str, Any] by using the sphinx format.

@MVrachev
Copy link
Collaborator Author

MVrachev commented Jan 4, 2022

I realized I was mistaken about the custom property, it's both documented and type annotated.
The only question remaining is are we sure the custom type is Any or it's actually Dict[str, Any]?
@jku what do you think?

@MVrachev MVrachev marked this pull request as ready for review January 4, 2022 13:44
Comment on lines 1290 to 1292
"""Contains additional information about the TargetFile instance"""
return self.unrecognized_fields.get("custom")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By adding a docstring to the property RTD noticed custom and documented it.

@jku
Copy link
Member

jku commented Jan 5, 2022

The only question remaining is are we sure the custom type is Any or it's actually Dict[str, Any]?

I wouldn't object to either.

Specification (summarized by me):

CUSTOM: An object.

Object is never defined but does seem to mean JSON object -- a dictionary with string keys -- so annotating it as such seems reasonable. There's probably no need for us to validate custom in deserialization though.

tuf/api/metadata.py Outdated Show resolved Hide resolved
Dictionary.get() by default will return "None" if the key is not
found as documented in:
https://docs.python.org/3/library/stdtypes.html#dict.get
This means we don't get anything by passing the default type.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
By adding a docstring to the property RTD noticed custom and
documented it.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@ivanayov
Copy link
Collaborator

ivanayov commented Jan 6, 2022

@jku A valid json in python works for boolean and number strings as well, so in that case Dict might not fit. On the other hand it would be a bit strange to attach such non-informative value to TargetFile, unless someone decides to use if for a hacky implementation in a very specific corner-case. But should we still allow that or just consider it's useless and restrict to Dict? With the change it nicer to use, formally it's not Any anymore.

@MVrachev MVrachev force-pushed the custom-doc branch 2 times, most recently from 7cebbe2 to 0c97649 Compare January 6, 2022 16:43
@MVrachev
Copy link
Collaborator Author

MVrachev commented Jan 6, 2022

@jku A valid json in python works for boolean and number strings as well, so in that case Dict might not fit. On the other hand it would be a bit strange to attach such non-informative value to TargetFile, unless someone decides to use if for a hacky implementation in a very specific corner-case. But should we still allow that or just consider it's useless and restrict to Dict? With the change it nicer to use, formally it's not Any anymore.

You are correct that custom could be a List and boolean, string and an int as well.
We are not sure what it could be in this case even though Dict makes the most sense.
I removed the commit that changes the type annotation.

@jku jku merged commit 4917a5c into theupdateframework:develop Jan 7, 2022
@MVrachev MVrachev deleted the custom-doc branch January 7, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metadata API: TargetFile.custom is not visible in documentation
4 participants