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

Make some helper functions for determining hashed bins part of repository_lib #1058

Merged

Conversation

joshuagl
Copy link
Member

Fixes issue #: N/A

Description of the changes being introduced by the pull request:

In order to support users of the WIP low-level API for updating individual metadata on addition of a target (see #1048), specifically Warehouse and PEP 458, make some of the helper functions for determining the appropriate bin for a given target part of the public API in repository_lib.

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

Remove the non-public function _get_hash() from repository_tool in favour
of the public function get_target_hash() in repository_lib

Signed-off-by: Joshua Lock <jlock@vmware.com>
@joshuagl
Copy link
Member Author

TODO: these functions don't yet have explicit tests (though they are tested implicitly by the delegate_hashed_bins tests)




def find_bin_for_hash(path_hash, number_of_bins):
Copy link
Member Author

Choose a reason for hiding this comment

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

Should perhaps be find_bin_for_target_hash() for better consistency with get_target_hash()

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Thanks for relocating the code, @joshuagl! I support your find_bin_for_target_hash rename suggestion.

Also, I'm for moving on without adding explicit tests. The delegate_hashed_bins tests cover the functionality sufficiently for the time being. And we can still add tests when we set the public API in stone. What do you think?

Move repository_tool._find_bin_for_hash() and helper functions it uses to
non-protected functions in repository_lib.
_find_bin_for_hash() becomes find_bin_for_target_hash()

These functions will be useful to adopters using the WIP low-level API for
updating metadata files (see theupdateframework#1048)

Signed-off-by: Joshua Lock <jlock@vmware.com>
@joshuagl
Copy link
Member Author

Thanks for relocating the code, @joshuagl! I support your find_bin_for_target_hash rename suggestion.

Pushed an update with the rename in ed45fb2

Also, I'm for moving on without adding explicit tests. The delegate_hashed_bins tests cover the functionality sufficiently for the time being. And we can still add tests when we set the public API in stone. What do you think?

I think this makes sense, thanks @lukpueh

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

🎉

@lukpueh lukpueh merged commit de8649e into theupdateframework:develop Jun 30, 2020
@joshuagl joshuagl deleted the joshuagl/protected2public branch June 30, 2020 09:44
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.

None yet

2 participants