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

Fix handling of create_git_blob's result #2122

Closed
wants to merge 1 commit into from
Closed

Fix handling of create_git_blob's result #2122

wants to merge 1 commit into from

Conversation

xmo-odoo
Copy link
Contributor

@xmo-odoo xmo-odoo commented Dec 2, 2021

create_git_blob does not actually return the entire blob object content, per the OpenAPI description it only returns a short-blob with the sha and url properties whereas a "full" blob also has encoding, content, and size (and node_id but that's not usually relevant).

Fix by marking the GitBlob returned as non-complete, this way normal completion will fetch the additional data iff it is accessed.

However testing github's behaviour raises a few more questions here: testCreateGitBlob specifies the latin1 encoding, per github's documentation that's not actually supported but apparently github simply treats anything other than "base64" as "utf-8", which is really "text": the json string gets encoded to utf-8 and stored as-is in the blob. So that test's encoding is really misleading.

I figure the interface could be improved there, but it's not entirely clear to me whether PyGithub is to provide a shallow Python interface to Github or possibly a more high-level one e.g. one could imagine a design where create_git_blob would automatically select between encoding depending on the input type (str | bytes) thus making the explicit encoding unnecessary, the encoding could also be an Enum and default to text-equivalent ("utf-8") with base64 being opt-in.

An other annoyance with the interface is it looks like github always returns base64-encoded content, again depending on the suitability of higher-level API GitBlob could have a utility property handling that automatically rather than requiring the user be aware of the issue.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@xmo-odoo
Copy link
Contributor Author

xmo-odoo commented Aug 8, 2022

:(

@xmo-odoo
Copy link
Contributor Author

@sfdye would you mind taking a look and e.g. reopening this PR? It still seems to be an issue in the current master, which makes using create_git_blob a lot harder than necessary.

@EnricoMi EnricoMi reopened this Mar 24, 2023
@stale stale bot removed the stale label Mar 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.08 🎉

Comparison is base (9f52e94) 98.68% compared to head (6047fb2) 98.77%.

❗ Current head 6047fb2 differs from pull request most recent head 4cdbea7. Consider uploading reports for the commit 4cdbea7 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2122      +/-   ##
==========================================
+ Coverage   98.68%   98.77%   +0.08%     
==========================================
  Files         117      117              
  Lines       11825    11674     -151     
==========================================
- Hits        11670    11531     -139     
+ Misses        155      143      -12     
Impacted Files Coverage Δ
github/Repository.py 97.20% <100.00%> (+<0.01%) ⬆️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xmo-odoo
Copy link
Contributor Author

Ah crap I updated using a merge commit, I'll have to fix that up, sorry about the noise.

@xmo-odoo
Copy link
Contributor Author

ok should be good now.

`create_git_blob` does not actually return the entire blob object
content, per the OpenAPI description it only returns a `short-blob`
with the `sha` and `url` properties whereas a "full" blob also has
`encoding`, `content`, and `size` (and `node_id` but that's not
relevant).

Fix by marking the `GitBlob` returned as non-complete, this way normal
completion will fetch the additional data iff it is accessed.
@xmo-odoo xmo-odoo closed this Oct 19, 2023
@xmo-odoo xmo-odoo deleted the create-git-blob-fetch branch October 19, 2023 11:33
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