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

Fixes #2143 - Support listing branch/pr per commit #2315

Merged
merged 7 commits into from
Dec 8, 2022

Conversation

zacdirect
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #2315 (faae216) into main (15887d9) will decrease coverage by 0.30%.
The diff coverage is 23.58%.

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

@@            Coverage Diff             @@
##             main    #2315      +/-   ##
==========================================
- Coverage   65.99%   65.68%   -0.31%     
==========================================
  Files         554      555       +1     
  Lines       14519    14625     +106     
  Branches      857      857              
==========================================
+ Hits         9582     9607      +25     
- Misses       4779     4860      +81     
  Partials      158      158              
Impacted Files Coverage Δ
Octokit/Helpers/AcceptHeaders.cs 100.00% <ø> (ø)
Octokit/Models/Response/CommitPullRequest.cs 0.00% <0.00%> (ø)
Octokit/Clients/RepositoryCommitsClient.cs 84.37% <50.00%> (-15.63%) ⬇️
...tive/Clients/ObservableRepositoryCommitsClients.cs 88.40% <60.00%> (-11.60%) ⬇️
Octokit/Helpers/ApiUrls.cs 97.33% <75.00%> (-0.20%) ⬇️

@zacdirect
Copy link
Contributor Author

I'm happy to make any adjustments. The one major thing I was on the fence about was creating a new model for CommitPullRequests. It could easily be returned as the existing PullRequest model, but in sampling some responses from GitHub I was never getting information back for the properties I removed. If their absence is possibly explained by a different preview header or license level then it would make sense to return them as the existing PullRequest model. I was unable to find any documentation explaining the difference so I opted for a new model to make it clear those PullRequest properties will never be there for these calls.

@github-actions
Copy link

'👋 Hey Friends, this pull request has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!'

@github-actions github-actions bot added the Stale label May 10, 2022
@github-actions github-actions bot closed this May 18, 2022
@zacdirect
Copy link
Contributor Author

zacdirect commented Oct 11, 2022 via email

@timrogers
Copy link
Contributor

Is this repo still active?

Yes!

@matt-richardson
Copy link
Contributor

Can we re-open this PR and look to get it merged?

@kfcampbell kfcampbell reopened this Oct 24, 2022
@kfcampbell
Copy link
Member

I've reopened the PR and fixed the merge conflict.

@github-actions github-actions bot removed the Stale label Oct 25, 2022
@nickfloyd nickfloyd added the Type: Feature New feature or request label Oct 27, 2022
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Everything looks good, thank you for these contributions! ❤️ . I just left one comment about the preview header. Not a show stopper, but it's essentially dead code and unnecessary, given REST API previews were deprecated last year. We can remove the code in this PR or do a follow-up if needed.

@@ -13,5 +13,7 @@ public static class AcceptHeaders
/// </summary>
/// <remarks>https://developer.github.com/v3/repos/contents/#custom-media-types</remarks>
public const string RawContentMediaType = "application/vnd.github.v3.raw";

public const string ListBranchOrPullForCommitPreview = "application/vnd.github.groot-preview+json";
Copy link
Contributor

Choose a reason for hiding this comment

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

Previews were deprecated last year for the REST API, so this header is no longer needed.

Copy link
Member

Choose a reason for hiding this comment

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

I've removed the offending header!

@matt-richardson
Copy link
Contributor

Hi @nickfloyd - is there anything stopping this one from being merged?

@nickfloyd
Copy link
Contributor

Hi @nickfloyd - is there anything stopping this one from being merged?

I think it's g2g. Let me give it a once over and get it merged in. Apologies for the delay.

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

@nickfloyd nickfloyd merged commit 84d44dc into octokit:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants