Description of issue or feature request:
The status()-method on the repository_tool.Repository-class, wraps repository_lib._log_status_of_top_level_roles, and logs status information about the repository.
Current behavior:
This is a non-comprehensive list of concerns identified in these functions:
-
status is a non-intuitive name for a function that changes and then logs the changed status. E.g. when calling status on unsigned metadata, status signs the metadata and informs you about the existence of those signatures.
-
The control flow is unclear, e.g. _log_status_of_top_level_roles may log the following information for any top level role:
(1) "ROLENAME role contains K / N public keys"
(2) "ROLENAME role contains K / N signing keys"
(3) "ROLENAME role contains K / N signatures"
However the information is shown:
(1) for roles that have K < N public keys,
(2) for roles that have K >= N public keys, and no signatures, and K < N signing keys,
(3) for roles that are processed before _generate_and_write_metadata and _log_status fails for a role, including the role that failed. The order of processed roles is: root, targets, snapshot, timestamp.
-
There is unnecessarily complicated and repetitive code, e.g.:
# This construct is repeated for every role
root_is_dirty = None
if 'root' in dirty_rolenames:
root_is_dirty = True
else:
root_is_dirty = False
...should be ...
root_is_dirty = False
if 'root' in dirty_rolenames:
root_is_dirty = True
... or just ...
root_is_dirty = 'root' in dirty_rolenames
And since this construct is repeated for every top-level role, the best way would be a helper function.
-
The separation of status and _log_status_of_top_level_roles function is not obvious.
status creates a temporary directory and passes it to _log_status_of_top_level_roles so that metadata created in _log_status_of_top_level_roles is written to the temporary directory. After logging the status, the temporary directory is removed. status is the only function that calls _log_status_of_top_level_roles.
-
The code documentation does not suffice to understand if the implementation matches the intended behavior.
Expected behavior:
- Identify intended behavior
- Update code documentation to clearly convey intended behavior
- Update implementation to match code documentation (if necessary)
- Simplify code (strive for readability!!)
Description of issue or feature request:
The
status()-method on therepository_tool.Repository-class, wrapsrepository_lib._log_status_of_top_level_roles, and logs status information about the repository.Current behavior:
This is a non-comprehensive list of concerns identified in these functions:
statusis a non-intuitive name for a function that changes and then logs the changed status. E.g. when callingstatuson unsigned metadata,statussigns the metadata and informs you about the existence of those signatures.The control flow is unclear, e.g.
_log_status_of_top_level_rolesmay log the following information for any top level role:(1)
"ROLENAME role contains K / N public keys"(2)
"ROLENAME role contains K / N signing keys"(3)
"ROLENAME role contains K / N signatures"However the information is shown:
(1) for roles that have
K < Npublic keys,(2) for roles that have
K >= Npublic keys, and no signatures, andK < Nsigning keys,(3) for roles that are processed before
_generate_and_write_metadataand_log_statusfails for a role, including the role that failed. The order of processed roles is: root, targets, snapshot, timestamp.There is unnecessarily complicated and repetitive code, e.g.:
...should be ...
... or just ...
And since this construct is repeated for every top-level role, the best way would be a helper function.
The separation of
statusand_log_status_of_top_level_rolesfunction is not obvious.statuscreates a temporary directory and passes it to_log_status_of_top_level_rolesso that metadata created in_log_status_of_top_level_rolesis written to the temporary directory. After logging the status, the temporary directory is removed.statusis the only function that calls_log_status_of_top_level_roles.The code documentation does not suffice to understand if the implementation matches the intended behavior.
Expected behavior: