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 type of commit for create_file #2120

Closed
wants to merge 1 commit into from
Closed

Fix type of commit for create_file #2120

wants to merge 1 commit into from

Conversation

xmo-odoo
Copy link
Contributor

The commit key of create_file's result currently uses github.Commit.Commit, but that does not match the data model of the endpoint: while ill documented on docs.github.com, looking at the published schema (via https://github.com/github/rest-api-description or more readably https://github.com/octokit/types.ts which is automatically generated from the former) file-commit.commit looks significantly more like a git-commit than a commit:

  • commit's author and committer are github objects (nullable-simple-user), while file-commit.commit's are git objects (triplets of name, email, and date)
  • commit embeds the tree, and message in a sub-object, while file-commit.commit has them at the toplevel
  • file-commit.commit has no stats or files properties, or comments_url

In all of these, file-commit.commit matches git-commit exactly.

The only real divergence between git-commit and file-commit.commit is that the schema indicates all of file-commit.commit's properties are optional, but I think it's a case of the endpoint being under-specified and the properties not being marked required, rather than them being explicitly optional. See github/rest-api-description#650 for an issue on that subject with more information.

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021

Codecov Report

Patch coverage has no change and project coverage change: +0.08 🎉

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

❗ Current head e96460b differs from pull request most recent head 405e668. Consider uploading reports for the commit 405e668 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    #2120      +/-   ##
==========================================
+ 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% <ø> (+<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.

Copy link
Collaborator

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

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

This looks amazing, and thank you for the brilliant detective work! I think part of this failure is that testCreateFile in tests/Repository.py does not check the return value of create_file() at all -- are you able to also correct that in this PR?

@xmo-odoo
Copy link
Contributor Author

xmo-odoo commented Dec 2, 2021

@s-t-e-v-e-n-k updated with a bit of checking in the test but wasn't quite sure what to test, so I just checked if the raw data matched the GitCommit, and if it !matched Commit.

I wanted to take a look at the tree property, but that requires adding new github responses and as noted in #2118 I'm not quite sure what the "proper" way to add them is.

@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

@sfdye could I bother you with reopening and taking a gander at this? As far as I can tell it remains an issue, which makes create_file difficult to use: the documentation is misleading, and the user needs to go and re-fetch a Commit or GitCommit to get a fully hydrated object even though github returned all the relevant information.

@EnricoMi EnricoMi reopened this Mar 24, 2023
@stale stale bot removed the stale label Mar 24, 2023
@EnricoMi
Copy link
Collaborator

@xmo-odoo I have rebased your branch to the latest maser

@xmo-odoo
Copy link
Contributor Author

@EnricoMi thanks, that'll avoid me making the mistake I made on #2122 which I need to fix.

The `commit` key of `create_file`'s result currently uses
`github.Commit.Commit`, but that does not match the data model of the
endpoint: while ill documented on docs.github.com, looking at the
published schema (via https://github.com/github/rest-api-description
or more readably https://github.com/octokit/types.ts which is
automatically generated from the former) `file-commit.commit` looks
significantly more like a `git-commit` than a `commit`:

* `commit`'s author and committer are git*hub* objects
  (`nullable-simple-user`), while `file-commit.commit`'s are git
  objects (triplets of name, email, and date)
* `commit` embeds the tree, and message in a sub-object, while
  `file-commit.commit` has them at the toplevel
* `file-commit.commit` has no `stats` or `files` properties, or
  `comments_url`

In all of these, `file-commit.commit` matches `git-commit` exactly.

The only real divergence between `git-commit` and `file-commit.commit`
is that the schema indicates all of `file-commit.commit`'s properties
are optional, but I think it's a case of the endpoint being
under-specified and the properties *not* being marked required, rather
than them being explicitly optional. See
github/rest-api-description#650 for an issue
on that subject with more information.
@xmo-odoo xmo-odoo closed this Oct 19, 2023
@xmo-odoo xmo-odoo deleted the create-file-commit 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

4 participants