[Approved] Add repository topics #2246
Conversation
Guided by tests. Thanks, Octokit tests!
Codecov Report
@@ Coverage Diff @@
## main #2246 +/- ##
==========================================
+ Coverage 65.92% 65.99% +0.07%
==========================================
Files 553 554 +1
Lines 14469 14519 +50
Branches 846 857 +11
==========================================
+ Hits 9538 9582 +44
- Misses 4773 4779 +6
Partials 158 158
|
|
Converting this from a draft PR as I'd love feedback on the progress so far and some discussion around the items I raise. (I know it needs tests; suggestions on the best places to focus my efforts there are also welcome.) |
|
Pinging @ryangribble @shiftkey for a quick pass on this when you get the chance (I think you're who I should ping for this sort of thing; if not, please feel free to tag others in. And sorry if the pinging is unwelcome! I couldn't find guidance on that.) I'll continue plodding forward, but I've left some review comments with questions / potential concerns, and I'd love feedback on where to focus the tests though I'll work to suss that out in the meantime. |
|
I see after making some updates that I need to ensure I can accept (I absolutely love these kinds of tests that help provide guardrails for someone like me cotributing changes -- thank you!!) |
|
This would be great to have! |
|
Keen to see this support added if it's possible 😄 subscribing for updates! |
|
Hey all -- bumping this to put it back on the radar. If the PR is too big or is taking too much mental effort to review, I can totally understand that and I'd be happy to restructure to break things up in a better way if possible. Any feedback appreciated. 👍 And if it's just that you remain swamped / a little burned out, that's fine and completely understandable as well. |
| { | ||
| readonly IGitHubClient _github = Helper.GetAuthenticatedClient(); | ||
| readonly RepositoryTopics _defaultTopics = new RepositoryTopics(new List<string> { "blog", "ruby", "jekyll" }); | ||
| const string theRepoOwner = "SeanKilleen"; |
There was a problem hiding this comment.
My memory is a bit hazy, but I think there are test helpers for creating a repository. That way an integration can create a repository, do the test stuff, then delete the repository at the end.
There was a problem hiding this comment.
Thanks for your persistence on this @SeanKilleen, and apologies for the delay in getting to this.
Only suggestions I have are about the integration test setup, everything else looks good to me.
|
@shiftkey no worries! Thanks for reviewing. Comments make sense to me and I appreciate you pointing me in the right spot for the helper code. Integration tests will be fixed up ASAP. |
|
@shiftkey I've made the changes to add and use the context -- thank you! Looks like there is a conflict with the method signature for the repository constructor. Going to fix it up now and hopefully we'll be good to go shortly. |
SeanKilleen
changed the title
|
@shiftkey OK, I think this is ready from my end once the build passes. |
|
@shiftkey no rush, but following up to say the build has passed so after a cursory review, I think this should be ready to go. Thanks for your follow-up! |
|
release_notes: Add support for repository topics API |
nickfloyd
added
Type: Feature
Resolves #1707.
Supersedes #1721
Will use #1721 as a reference but doing the steps myself to gain better experience/understanding of the codebase.
Comments and suggestions welcome along the way!
Work Remaining
Response Class Support
New API Calls
Support Search Repositories by Topic
https://help.github.com/articles/searching-repositories/#search-by-repository-topic
Tests
RepositoryTopicsClean up integration tests -- remove double-checks?IDisposablein favor of another technique?Other stuff discovered as I progressed
mercypreview is enforced -- thePreviewAttributeis metadata, which I'd misunderstood.