Codecov Report

Merging#2396 (8751402) intomain (0ecf3bb) willdecrease coverage by0.04%.
The diff coverage is76.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 asUTF-8 text.

So I propose to add a methodf.text(encoding='UTF-8') (similar toRequests'st.text) so you could change the encoding but most users could just dof.text().

For bytes I would personally prefer havingf.bytes() methods to avoid usingdecode 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 likeUTF-8.

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

PS I've never usedencoding = '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 torequests/httpx and similar underlying libraries.

For context, thisdecode() 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 in2eac071).

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 itactually convenient for the user ;) I agreedecode() isn't that clear though.

Edit: theencoding 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 ofdecodes we're calling so maybe we shouldn't mix them. I'll rework this a bit :)