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

Move all hash checking to hash_matches #157

Merged
merged 4 commits into from
Apr 2, 2020
Merged

Conversation

leouieda
Copy link
Member

@leouieda leouieda commented Apr 1, 2020

Including the exception raising controlled by new parameter strict.
This removes the need for re-calculating the new hash just so we can
include it in the error message. The hash calculation can be heavy
for large files so we don't want to do it twice. Also improve the error
message to include the hash algorithm and suggest what went wrong.

Working towards #152

Reminders:

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst and the base __init__.py file for the package.
  • Write detailed docstrings for all functions/classes/methods. It often helps to design better code if you write the docstrings first.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.
  • Add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.

Including the exception raising controlled by new parameter `strict`.
This removes the need for re-calculating the new hash just so we can
include it in the error message. The hash calculation can be heavy
for large files so we don't want to do it twice. Also improve the error
message to include the hash algorithm and suggest what went wrong.
@leouieda leouieda requested a review from danshapero April 1, 2020 18:09
Forgot to include in the check and had misformatted error message
@danshapero
Copy link
Contributor

I'm assuming that the strict = False case will get used for #152. If you wanted to be really obsessive about branch coverage then you could add an explicit test for that but I don't think it's necessary.

@leouieda
Copy link
Member Author

leouieda commented Apr 2, 2020

@danshapero right, I forgot to add the extra tests to this PR. I'll add some explicit tests for this function (right now it gets tested as part of fetch).

Both cases are used within fetch: strict=False when checking for updates and stritc=True for verifying downloaded files. My plan for #152 was to allow known_hash=None which would always result in a True return value. That way bypassing the hash check would only require changing this one function.

@leouieda
Copy link
Member Author

leouieda commented Apr 2, 2020

Thanks for the review, by the way 🙂 Much appreciated

@leouieda leouieda merged commit 4f66acf into master Apr 2, 2020
@leouieda leouieda deleted the hash_matches_strict branch April 2, 2020 16:24
leouieda added a commit that referenced this pull request Apr 3, 2020
Fixes a regression introduced in #157 where we started including the
name of the temporary download file in the exception again. However,
since the download is deleted in case of hash mismatch, it's also not
very useful to print the name of a file that doesn't exist. Instead, I
included the download URL in the exception so that a user can download
for themselves and check.
leouieda added a commit that referenced this pull request Apr 8, 2020
Fixes a regression introduced in #157 where we started including the
name of the temporary download file in the exception again. Revert back
to including the name of the file in the registry.
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.

2 participants