-
Notifications
You must be signed in to change notification settings - Fork 673
fix(api): head requests for projectfilemanager #2977
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
Conversation
Also looks like it needs to be rebased against the current |
So looks like an existing functional test is failing because it expects the return value of headers to be a dict rather than a https://github.com/python-gitlab/python-gitlab/blob/main/tests/functional/api/test_repository.py#L43 |
@holysoles our client's But I'm a bit confused our existing tests work as intended as opposed to your issue description, it seems like the issue there is actually your use of keyword args, but I'll have to check in more detail when I get home. |
@nejch now that you say that, the keyword args do seem like the problem. I think my confusion initially stemmed from the official REST API docs indicate that there isnt a difference in how the API call should be made, but here, the user needs to pass Given all this, does it make sense to not transform the headers dict and just return it, but still proceed with the other changes so the consistency of the parameter name can be improved? |
Yes, I think that makes sense. TBH I'm questioning whether we really need to even be overriding the For now IMO best to just align the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2977 +/- ##
=======================================
Coverage 96.60% 96.61%
=======================================
Files 95 95
Lines 6072 6080 +8
=======================================
+ Hits 5866 5874 +8
Misses 206 206
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @holysoles for the PR and the rework, just had a small nit in the docstring that I applied directly so we can go ahead with this.
Changes
Closes #2976
The
ProjectFileManager
class current has ahead
method, which throws a 404 error when used, as it doesn't have the same logic for URL path construction that itsget
method has. This is because it is inheriting fromGetMixin
, which inherits fromHeadMixin
. The benefit of having the HEAD request properly support is you can inspect and compare the file without needing to fully download it, as the actual file content is not returned.This change:
ProjectFileManager
inheritance ofGetMixin
, since we're not making proper use of that class.head
method toProjectFileManager
that correctly constructs the API queryhead
method based on mocked real-world headersNote that because the HEAD request returns the requested data as headers and not a JSON object, we need to map the keys to the expected ones for
ProjectFile
. I did this in a dynamic way, but happy to re-implement it with a hardcoded map of expected values.Documentation and testing
Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated: