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

Metadata API: Remove 3 'update' methods + tests #1736

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Dec 20, 2021

Fixes #1627

Description of the changes being introduced by the pull request:
Remove ambiguous, unspecific, opinionated and trivial 'update' methods, which can be replaced by feasible one-liners that assign values directly to the object attribute to be updated. (see #1627 for details).

Reasons to have these methods would be increased usability in terms of

  • reduced work
  • immediate feedback on invalid assignments

However, given above described issues, the reasons against the methods as they are now seem to outweigh the reasons for them. Furthermore, it seems easier to re-add similar methods, which addressed these issues, after the upcoming 1.0.0 release than to remove or modify them.

This patch also removes the corresponding tests as they become irrelevant (there is no need to test object assignment). In the case of the timestamp test, the removal also includes redundant test logic, which is already tested in test_metadata_base.

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 20, 2021

Pull Request Test Coverage Report for Build 1605943813

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 97.699%

Totals Coverage Status
Change from base Build 1605803389: -0.003%
Covered Lines: 4089
Relevant Lines: 4169

💛 - Coveralls

Remove ambiguous, unspecific, opinionated and trivial 'update'
methods, which can be replaced by feasible one-liners that assign
values directly to the object attribute to be *updated*. (see theupdateframework#1627
for details).

Reasons to have these methods would be increased usability in terms of
- reduced work
- immediate feedback on invalid assignments

However, given above described issues, the reasons against the
methods as they are now seem to outweigh the reasons for them.
Furthermore, it seems easier to re-add similar methods, which
addressed these issues, after the upcoming 1.0.0 release than to
remove or modify them.

This patch also removes the corresponding tests as they become
irrelevant (there is no need to test object assignment).  In the
case of the timestamp test, the removal also includes redundant
test logic, which is already tested in `test_metadata_base`.

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh force-pushed the rm-metadata-api-update branch from 5131962 to f22f357 Compare December 21, 2021 08:49
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Looks correct to me -- maybe we find out later that we do want some helper functions... but they will almost certainly look different so doing this ASAP seems like the right move.

the bump_*() methods have a bit of the same issue (they are not easier to use than just accessing the data structure, and bump_expiry does not do what I think repositories would want to do) but this is a move in the right direction.

@lukpueh
Copy link
Member Author

lukpueh commented Dec 21, 2021

the bump_*() methods have a bit of the same issue (they are not easier to use than just accessing the data structure, and bump_expiry does not do what I think repositories would want to do) but this is a move in the right direction.

Agreed. See for instance the basic repo example. There, for version bump I do the addition assignment myself...

roles["targets"].signed.version += 1

... and for expiration "bump"s, I use a custom oneliner helper, because I needed something similar for the expires constructor argument.

def _in(days: float) -> datetime:
"""Adds 'days' to now and returns datetime object w/o microseconds."""
return datetime.utcnow().replace(microsecond=0) + timedelta(days=days)

roles[delegatee_name] = Metadata[Targets](
signed=Targets(
version=1,
spec_version=SPEC_VERSION,
expires=_in(7),
targets={target_path: target_file_info},
),
signatures=OrderedDict(),
)

roles["targets"].signed.expires = _in(365)

@lukpueh
Copy link
Member Author

lukpueh commented Dec 21, 2021

I'll create a ticket for those two bump methods...

@lukpueh lukpueh merged commit cc2326d into theupdateframework:develop Dec 21, 2021
@lukpueh
Copy link
Member Author

lukpueh commented Dec 21, 2021

I'll create a ticket for those two bump methods...

Just saw, there already is one for expiry #1727. And for bump_version I'll just drop a fix right away.

@coveralls
Copy link

coveralls commented Dec 27, 2024

Pull Request Test Coverage Report for Build 1605943813

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.0%) to 98.69%

Totals Coverage Status
Change from base Build 1605803389: 1.0%
Covered Lines: 3928
Relevant Lines: 3950

💛 - Coveralls

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: Consider removing 'update()' methods
3 participants