[META] Cleaning up obsoleted items

[ryangribble]

Just opening a discussion to review the items marked [Obsolete], since most of them will have been more than 3 months old by the time of the next release (and many of them are 1 year+ old too)... it must be time to 🚀 🔥 some of them 😀

We've currently got 80 occurences across 39 files, and by my (manual count) about 14 actual functional areas.

Here's a list of the areas and the number of days since they were obsoleted

IAuthorizationsClient.RevokeAllApplicationAuthentications() 43 days
IOrganizationMembersClient.GetAll(string org, string filter) 454 days 
IRepositoriesClient.CommitStatus 83 days
IRepositoryContentsClient.GetArchiveLink() 289 days
ISshKeysClient and associated SshKey class, helpers etc 29 days
ITeamsClient.IsMember() 299 days

GitHubClient.Release 82 days
GitHubClient.GitDatabase 82 days

ApiConnection.GetRedirect() 98 days
ApiUrls.Members() 454 days
IConnection.Get<T>(... allowAutoRedirect) 290 days

SearchIssuesRequest ctor(term, owner, name) 241 days

Repository.SubscribersCount 378 days
Repository.Organization 384 days

And here's the raw "Find in Files" output

  \Octokit\Clients\AuthorizationsClient.cs(384):        [Obsolete("This feature is no longer supported in the GitHub API and will be removed in a future release")]
  \Octokit\Clients\IAuthorizationsClient.cs(233):        [Obsolete("This feature is no longer supported in the GitHub API and will be removed in a future release")]
  \Octokit\Clients\IOrganizationMembersClient.cs(66):        [Obsolete("No longer supported, use GetAll(string, OrganizationMembersFilter) instead")]
  \Octokit\Clients\IRepositoriesClient.cs(198):        [Obsolete("Use Status instead")]
  \Octokit\Clients\IRepositoryContentsClient.cs(104):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit\Clients\IRepositoryContentsClient.cs(127):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit\Clients\IRepositoryContentsClient.cs(152):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit\Clients\ISshKeysClient.cs(22):        [Obsolete("This method is obsolete. Please use User.Keys.Get(int) instead.")]
  \Octokit\Clients\ISshKeysClient.cs(30):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll(string) instead.")]
  \Octokit\Clients\ISshKeysClient.cs(40):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll() instead.")]
  \Octokit\Clients\ISshKeysClient.cs(49):        [Obsolete("This method is obsolete. Please use User.Keys.Create(NewPublicKey) instead.")]
  \Octokit\Clients\ISshKeysClient.cs(59):        [Obsolete("This method is no longer supported in the GitHub API. Delete and Create the key again instead.")]
  \Octokit\Clients\ISshKeysClient.cs(68):        [Obsolete("This method is obsolete. Please use User.Keys.Delete(int) instead.")]
  \Octokit\Clients\ITeamsClient.cs(106):        [Obsolete("Use GetMembership(id, login) as this will report on pending requests")]
  \Octokit\Clients\OrganizationMembersClient.cs(118):        [Obsolete("No longer supported, use GetAll(string, OrganizationMembersFilter) instead")]
  \Octokit\Clients\RepositoriesClient.cs(320):        [Obsolete("Use Status instead")]
  \Octokit\Clients\RepositoriesClient.cs(384):        [Obsolete("Commit information is now available under the Commit property. This will be removed in a future update.")]
  \Octokit\Clients\RepositoriesClient.cs(425):        [Obsolete("Comment information is now available under the Comment property. This will be removed in a future update.")]
  \Octokit\Clients\RepositoryContentsClient.cs(161):        [Obsolete("Octokit's HTTP library now follows redirects by default - this API will be removed in a future release")]
  \Octokit\Clients\RepositoryContentsClient.cs(190):        [Obsolete("Octokit's HTTP library now follows redirects by default - this API will be removed in a future release")]
  \Octokit\Clients\RepositoryContentsClient.cs(221):        [Obsolete("Octokit's HTTP library now follows redirects by default - this API will be removed in a future release")]
  \Octokit\Clients\SshKeysClient.cs(30):        [Obsolete("This method is obsolete. Please use User.Keys.Get(int) instead.")]
  \Octokit\Clients\SshKeysClient.cs(43):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll(string) instead.")]
  \Octokit\Clients\SshKeysClient.cs(56):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll() instead.")]
  \Octokit\Clients\SshKeysClient.cs(68):        [Obsolete("This method is obsolete. Please use User.Keys.Create(NewPublicKey) instead.")]
  \Octokit\Clients\SshKeysClient.cs(83):        [Obsolete("This method is no longer supported in the GitHub API. Delete and Create the key again instead.")]
  \Octokit\Clients\SshKeysClient.cs(98):        [Obsolete("This method is obsolete. Please use User.Keys.Delete(int) instead.")]
  \Octokit\Clients\TeamsClient.cs(218):        [Obsolete("Use GetMembership(id, login) as this will report on pending requests")]
  \Octokit\GitHubClient.cs(224):        [Obsolete("Use Repository.Release instead")]
  \Octokit\GitHubClient.cs(263):        [Obsolete("Use Git instead")]
  \Octokit\Helpers\ApiExtensions.cs(77):        [Obsolete("Octokit's HTTP library now follows redirects by default - this API will be removed in a future release")]
  \Octokit\Helpers\ApiUrls.cs(418):        [Obsolete("No longer supported, use Members(string, OrganizationMembersFilter)")]
  \Octokit\Helpers\ModelExtensions.cs(23):        [Obsolete("This method will be removed in a future release.")]
  \Octokit\Helpers\ModelExtensions.cs(39):        [Obsolete("This method will be removed in a future release.")]
  \Octokit\Http\ApiConnection.cs(440):        [Obsolete("Octokit's HTTP library now follows redirects by default - this API will be removed in a future release")]
  \Octokit\Http\Connection.cs(170):        [Obsolete("allowAutoRedirect is no longer respected and will be deprecated in a future release")]
  \Octokit\Http\IConnection.cs(46):        [Obsolete("allowAutoRedirect is no longer respected and will be deprecated in a future release")]
  \Octokit\IGitHubClient.cs(94):        [Obsolete("Use Repository.Release instead")]
  \Octokit\IGitHubClient.cs(131):        [Obsolete("Use Git instead")]
  \Octokit\Models\Request\SearchIssuesRequest.cs(35):        [Obsolete("this will be deprecated in a future version")]
  \Octokit\Models\Response\Repository.cs(92):        [Obsolete("This property has been obsoleted. Please use WatchedClient.GetAllWatchers instead.")]
  \Octokit\Models\Response\Repository.cs(107):        [Obsolete("This property has been obsoleted by Repository.Owner. Please use Repository.Owner.Type instead.")]
  \Octokit\Models\Response\SshKey.cs(8):    [Obsolete("This response class is obsolete. Please use PublicKey instead")]
  \Octokit\Models\Response\SshKeyInfo.cs(8):    [Obsolete("This class will be removed in a future release.")]
  \ReleaseNotes.md(20):we're marking the endpoints as `[Obsolete]` and indicating the new location.
  \Octokit.Reactive\Clients\IObservableAuthorizationsClient.cs(212):        [Obsolete("This feature is no longer supported in the GitHub API and will be removed in a future release")]
  \Octokit.Reactive\Clients\IObservableOrganizationMembersClient.cs(59):        [Obsolete("No longer supported, use GetAll(string, OrganizationMembersFilter) instead")]
  \Octokit.Reactive\Clients\IObservableRepositoriesClient.cs(120):        [Obsolete("Use Status instead")]
  \Octokit.Reactive\Clients\IObservableRepositoriesClient.cs(155):        [Obsolete("Comment information is now available under the Comment property. This will be removed in a future update.")]
  \Octokit.Reactive\Clients\IObservableRepositoriesClient.cs(299):        [Obsolete("Collaborator information is now available under the Collaborator property. This will be removed in a future update.")]
  \Octokit.Reactive\Clients\IObservableRepositoryContentsClient.cs(37):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit.Reactive\Clients\IObservableRepositoryContentsClient.cs(60):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit.Reactive\Clients\IObservableRepositoryContentsClient.cs(85):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit.Reactive\Clients\IObservableSshKeysClient.cs(15):        [Obsolete("This method is obsolete. Please use User.Keys.Get(int) instead.")]
  \Octokit.Reactive\Clients\IObservableSshKeysClient.cs(23):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll(string) instead.")]
  \Octokit.Reactive\Clients\IObservableSshKeysClient.cs(33):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll() instead.")]
  \Octokit.Reactive\Clients\IObservableSshKeysClient.cs(42):        [Obsolete("This method is obsolete. Please use User.Keys.Create(NewPublicKey) instead.")]
  \Octokit.Reactive\Clients\IObservableSshKeysClient.cs(52):        [Obsolete("This method is no longer supported in the GitHub API. Delete and Create the key again instead.")]
  \Octokit.Reactive\Clients\IObservableSshKeysClient.cs(61):        [Obsolete("This method is obsolete. Please use User.Keys.Delete(int) instead.")]
  \Octokit.Reactive\Clients\IObservableTeamsClient.cs(104):        [Obsolete("Use GetMembership(id, login) to detect pending memberships")]
  \Octokit.Reactive\Clients\ObservableAuthorizationsClient.cs(297):        [Obsolete("This feature is no longer supported in the GitHub API and will be removed in a future release")]
  \Octokit.Reactive\Clients\ObservableOrganizationMembersClient.cs(86):        [Obsolete("No longer supported, use GetAll(string, OrganizationMembersFilter) instead")]
  \Octokit.Reactive\Clients\ObservableRepositoriesClient.cs(200):        [Obsolete("Use Status instead")]
  \Octokit.Reactive\Clients\ObservableRepositoriesClient.cs(235):        [Obsolete("Comment information is now available under the Comment property. This will be removed in a future update.")]
  \Octokit.Reactive\Clients\ObservableRepositoriesClient.cs(446):        [Obsolete("Collaborator information is now available under the Collaborator property. This will be removed in a future update.")]
  \Octokit.Reactive\Clients\ObservableRepositoriesClient.cs(463):        [Obsolete("Commit information is now available under the Commit property. This will be removed in a future update.")]
  \Octokit.Reactive\Clients\ObservableRepositoryContentsClient.cs(62):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit.Reactive\Clients\ObservableRepositoryContentsClient.cs(91):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit.Reactive\Clients\ObservableRepositoryContentsClient.cs(122):        [Obsolete("Use GetArchive to download the archive instead")]
  \Octokit.Reactive\Clients\ObservableSshKeysClient.cs(30):        [Obsolete("This method is obsolete. Please use User.Keys.Get(int) instead.")]
  \Octokit.Reactive\Clients\ObservableSshKeysClient.cs(41):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll(string) instead.")]
  \Octokit.Reactive\Clients\ObservableSshKeysClient.cs(54):        [Obsolete("This method is obsolete. Please use User.Keys.GetAll() instead.")]
  \Octokit.Reactive\Clients\ObservableSshKeysClient.cs(66):        [Obsolete("This method is obsolete. Please use User.Keys.Create(NewPublicKey) instead.")]
  \Octokit.Reactive\Clients\ObservableSshKeysClient.cs(81):        [Obsolete("This method is no longer supported in the GitHub API. Delete and Create the key again instead.")]
  \Octokit.Reactive\Clients\ObservableSshKeysClient.cs(95):        [Obsolete("This method is obsolete. Please use User.Keys.Delete(int) instead.")]
  \Octokit.Reactive\Clients\ObservableTeamsClient.cs(144):        [Obsolete("Use GetMembership(id, login) to detect pending memberships")]
  \Octokit.Reactive\IObservableGitHubClient.cs(18):        [Obsolete("Use Repository.Release instead")]
  \Octokit.Reactive\IObservableGitHubClient.cs(25):        [Obsolete("Use Git instead")]
  \Octokit.Reactive\ObservableGitHubClient.cs(65):        [Obsolete("Use Repository.Release instead")]
  \Octokit.Reactive\ObservableGitHubClient.cs(70):        [Obsolete("Use Git instead")]

[shiftkey]

Alright, quick brain dump:

• as Octokit is being used in many places, we should be [Obsoleting] before removing any APIs.
• the [Obsolete] comment should indicate what the caller needs to update
• for the most part, the code should continue to work while obsolete
  • allowAutoRedirect is one exception I can think of, but as we're all-in on redirects then I can live with that

The $64000 question of course is "how long should these be obsolete for before removing them?"

I might be being totally arbitrary here, but 2 releases feels like a good number - just in case callers happen to skip a release. We've been rather slow with releases lately (~1 month and ~2 months between the last three releases) but if we get moving to a more frequent cadence of releases (hint: yes, we should) then we can re-evaluate this and perhaps move to a time-based approach.

[M-Zuber]

brain fart: Should we include the release in the obsolete message? (the release that is current when the change is made || a reviewer can just say - add in "in release x.x.x)

[M-Zuber]

but 2 releases feels like a good number

I agree, but for devil's advocacy do accepted (& actually used) guidelines exist for this kind of thing?

[shiftkey]

Should we include the release in the obsolete message?

Maybe? Having the release number there may mean they'll go and read the release notes - but perhaps the details in the message are enough for them to understand...

I agree, but for devil's advocacy do accepted (& actually used) guidelines exist for this kind of thing?

"It depends" is really the best answer I can come up with, and this probably comes down to support lifecycles. On one hand, $vendor may need to support obsolete features in an API until the next major release. On the other hand, some OSS library may choose to ignore SemVer and arbitrarily introduce breaking changes because move fast and break stuff.

Technically we're pre-1.0 so SemVer is not very opinionated about this, but I'd favour being more conservative in general. In my mind 1.0 is having GitHub v3 API support covered and some necessary infrastructure changes in place - but we can act like post-1.0 right now with respect to obsoleting changes...

[M-Zuber]

Sounds good to me.
I have some Real Life™ to take care of, and then I will try and post back with a sorting of Obsolete methods based on release

[ryangribble]

So given that theres a pretty massive gap between the ones that are 200+ days since obsoleted, and a bunch sitting at around the ~80-100 mark... we should be pretty safe to remove all the really old stuff?

🔥 these

SearchIssuesRequest ctor(term, owner, name) 241 days
IRepositoryContentsClient.GetArchiveLink() 289 days
IConnection.Get<T>(... allowAutoRedirect) 290 days
ITeamsClient.IsMember() 299 days
Repository.SubscribersCount 378 days
Repository.Organization 384 days
IOrganizationMembersClient.GetAll(string org, string filter) 454 days 
ApiUrls.Members() 454 days

And keep the following for now

ISshKeysClient and associated SshKey class, helpers etc 29 days
IAuthorizationsClient.RevokeAllApplicationAuthentications() 43 days
GitHubClient.Release 82 days
GitHubClient.GitDatabase 82 days
IRepositoriesClient.CommitStatus 83 days
ApiConnection.GetRedirect() 98 days

Happy with that approach for .20 release?

[M-Zuber]

Happy with that approach for .20 release?

If the answer is yes, I would like to do that PR

[shiftkey]

Happy with that approach for .20 release?

Yes, very happy

[M-Zuber]

ApiConnection.GetRedirect() depends on IConnection.Get(... allowAutoRedirect)

It currently did this: return connection.Get<T>(uri, null, null, false);.
Should I just drop the last param?

[shiftkey]

@M-Zuber I think we should drop usages of the allowAutoRedirect parameter and possibly remove it - if that's already been marked as obsolete...

[ryangribble]

Im thinking that we should just 🔥 the ApiConnection.GetRedirect() function, since it was the only place that was using that Get overload anyway. I realise it was in my list to "keep" but at 110 days since obsolete, and given we are already removing the Connection related redirect flag (which isn't honoured anyway) I'm voting we sharpen the axe and just 🔥 all 3 ?

ApiConnection.GetRedirect()

IConnection.Task<IApiResponse<T>> Get<T>(Uri uri, IDictionary<string, string> parameters, string accepts, bool allowAutoRedirect)

Connection.Task<IApiResponse<T>> Get<T>(Uri uri, IDictionary<string, string> parameters, string accepts, bool allowAutoRedirect)

[github-actions]

👋 Hey Friends, this issue 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!