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

feat(files): allow decoding project files directly to string #2396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nejch
Copy link
Member

@nejch nejch commented Nov 26, 2022

Provides a slightly less clunky interface so people don't need to do f.decode.().decode("utf-8") in user code.

@codecov-commenter
Copy link

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     
Flag Coverage Δ
api_func_v4 83.71% <76.92%> (-0.02%) ⬇️
cli_func_v4 82.31% <46.15%> (-0.08%) ⬇️
unit 87.67% <46.15%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
gitlab/v4/objects/files.py 96.70% <76.92%> (-3.30%) ⬇️

@nejch nejch marked this pull request as ready for review November 26, 2022 15:38
@gdubicki
Copy link

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.

@nejch nejch assigned nejch and unassigned max-wittig Dec 4, 2022
@nejch
Copy link
Member Author

nejch commented Dec 4, 2022

@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.

@nejch
Copy link
Member Author

nejch commented Feb 7, 2023

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 :)

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.

None yet

4 participants