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

Calculate and store hash for url dependencies #7121

Merged
merged 5 commits into from Dec 7, 2022

Conversation

dunkmann00
Copy link
Contributor

@dunkmann00 dunkmann00 commented Nov 30, 2022

This performs the same hashing process as is done on file dependencies, for url dependencies. This requires python-poetry/poetry-core#535 to work.

I haven't updated the tests yet as I would first like to get feedback from the maintainers.

Pull Request Check List

Resolves: #7122

  • Added tests for changed code.
  • Updated documentation for changed code.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 30, 2022

thanks!

I don't think that attaching the hash() method to the url dependency is the right way:

  • you don't use self in that method at all, which is a clue that it doesn't particularly belong there
  • you haven't achieved any code re-use, this essentially duplicates the hash() method on the file dependency

I would suggest that the method on the file dependency be deprecated or removed, and you replace it with a standalone utility function that just takes a Path and calculates the hash.

Also you could put that utility in poetry proper rather than poetry-core, which will allow you to make the unit tests work without having cross-repository problems to work through

@dunkmann00
Copy link
Contributor Author

Okay, I moved hash() into utils.helpers and updated any calls to use this new function.

The tests still fail since the lockfile that's generated has the hashes in it and doesn't match the test fixtures. Is there a recommended way to generate the new fixture test files?

@dimbleby
Copy link
Contributor

Is there a recommended way to generate the new fixture test files?

With a grand total of four failures it shouldn't be very tedious just to fix up those testcases manually,

pytest -k <test-name> -vv will give a fairly readable diff to tell you what's changed so that's easy to copy into the test script (if you're happy with it)

@dunkmann00 dunkmann00 marked this pull request as ready for review November 30, 2022 20:39
@dunkmann00 dunkmann00 marked this pull request as draft November 30, 2022 20:57
@dunkmann00 dunkmann00 marked this pull request as ready for review November 30, 2022 23:17
@dunkmann00
Copy link
Contributor Author

Fixed test failures and added in tests for the new get_file_hash() function. The tests are pulled over from what used to be in poetry-core.

@dunkmann00 dunkmann00 force-pushed the urldeps-hash branch 2 times, most recently from 3de5f84 to 8c2113c Compare December 1, 2022 20:06
dimbleby
dimbleby previously approved these changes Dec 1, 2022
This performs the same hashing process as is done on file dependencies,
for url dependencies. This requires a change in `poetry-core` to work.
These tests were copied over from 'poetry-core' and updated to work
here.
Don't use FileDependency when validating the archive hash in executor,
just compute the hash with the archive path directly.
@radoering radoering merged commit 0ca8b7e into python-poetry:master Dec 7, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/pyproject Metadata/pyproject.toml-related impact/changelog Requires a changelog entry kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calculate and store hash for url dependencies
4 participants