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

Handle encoding value of "none" #2924

Merged
merged 6 commits into from Sep 18, 2023
Merged

Conversation

GusPrice
Copy link
Contributor

When a file is over 1 MB in a repository, GitHub responds with "none" as the encoding value. Handle this accordingly.

@GusPrice GusPrice changed the title Handle Encoding value of "none" Handle encoding value of "none" Sep 11, 2023
@WillAbides
Copy link
Contributor

If I understand the docs correctly, Content should always be nil when encoding is "none". I think it would be better to continue returning an error on "none", but it could be a better error that explains that this is because the file size is over 1MB and suggests using DownloadContents instead.

@GusPrice
Copy link
Contributor Author

Ah! It seems in fact I have misunderstood! Definitely would be nice to have a better error though. I'll update my PR accordingly.

@GusPrice
Copy link
Contributor Author

Updated to give a nicer error message in that case!

Copy link
Contributor

@WillAbides WillAbides left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a small nit about the test case, but you don't necessarily need to take my suggestion there.

To set expectations, the primary maintainer is out until 9/18, so this PR is not likely to be merged before then.

github/repos_contents_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #2924 (bee9a1a) into master (fd33b81) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head bee9a1a differs from pull request most recent head 6e8946c. Consider uploading reports for the commit 6e8946c to get more accurate results

@@           Coverage Diff           @@
##           master    #2924   +/-   ##
=======================================
  Coverage   98.17%   98.17%           
=======================================
  Files         143      143           
  Lines       12609    12611    +2     
=======================================
+ Hits        12379    12381    +2     
  Misses        156      156           
  Partials       74       74           
Files Changed Coverage Δ
github/repos_contents.go 87.97% <100.00%> (+0.15%) ⬆️

github/repos_contents.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @GusPrice and @WillAbides !
LGTM.
Merging.

@gmlewis gmlewis merged commit 9c58b7b into google:master Sep 18, 2023
8 checks passed
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
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

3 participants