Codecov Report

Merging #2396 (8751402) into main (0ecf3bb) will decrease coverage by 0.04%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #2396      +/-   ##
==========================================
- Coverage   95.97%   95.92%   -0.05%     
==========================================
  Files          80       80              
  Lines        5342     5354      +12     
==========================================
+ Hits         5127     5136       +9     
- Misses        215      218       +3     

Flags with carried forward coverage won't be shown. Click here to find out more.

Hi! Sorry if I am oversharing here, especially as I don't know this project and its conventions well (yet!).

While this is definitely an improvement from the previous syntax I feel that this could and maybe should be even simpler.

Because people mostly store source code in git then the most popular use-case here will probably be getting the content of the file as UTF-8 text.

So I propose to add a method f.text(encoding='UTF-8') (similar to Requests's t.text) so you could change the encoding but most users could just do f.text().

For bytes I would personally prefer having f.bytes() methods to avoid using decode keyword as for me it's almost never clear what it does as it depends on the context. Especially in GitLab Files API, where we have double encoding, first with base64 and then with a text encoding like UTF-8.

For completeness I would add f.base64(). This way you can have a single call to any form of the underlying file.

PS I've never used encoding = 'text' in GitLab Files API so I don't know - doesn't it cause the file to be stored without base64? If so, we should make sure that our helper functions deal with those 2 cases, base64-encoded files and not.

@gdubicki thanks for the feedback! I'll take another look at what wording makes the most sense, especially when compared to requests/httpx and similar underlying libraries.

For context, this decode() method is one of the few public methods in python-gitlab that are completely custom (i.e. not implementing an API endpoint) and here for the user's convenience rather than to mirror API functionality (this one introduced by the original author in 2eac071).

In most other cases we do not add more higher-level convenience and instead keep it a thin wrapper, so I'd be wary of introducing more methods but if we do already provide some decoding then it probably makes sense to make it actually convenient for the user ;) I agree decode() isn't that clear though.

Edit: the encoding arg on POST shouldn't have an effect on this, from what I can tell it's to indicate to GitLab the encoding of the content that is being sent in the payload. See discussion in #427.

Maybe an additional .text() really is more explicit. It's 2 totally different kinds of decodes we're calling so maybe we shouldn't mix them. I'll rework this a bit :)