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

Cleanup obsolete items #1224

Merged
merged 9 commits into from
Apr 14, 2016
Merged

Conversation

M-Zuber
Copy link
Contributor

@M-Zuber M-Zuber commented Mar 27, 2016

per #1194 (specifically this comment)

@ryangribble
Copy link
Contributor

one small item to draw attention to is this question

As per my reply on #1194 I suggest we should just 🔥 the ApiExtensions.GetRedirect() method

@M-Zuber
Copy link
Contributor Author

M-Zuber commented Mar 29, 2016

@ryangribble done

@ryangribble
Copy link
Contributor

Oops, it looks like there is one more reference in ApiConnection and IApiConnection with GetRedirect()

The problem is, the concrete implementation was made obsolete 111 days ago, but the interface member hasnt been marked obsolete at all! 😭

But if we have to keep this, then it means we have to keep the ApiExtension one around as well, but that seems kinda pointless since the GetRedirect() will never actually get a HTTP 302 response anyway and thus will always throw an exception

@shiftkey what do you reckon to 🔥ing IApiConnection.GetRedirect(), eventhough it wasnt marked obsolete? Note that the concrete implementation, as well as multiple other places regarding Redirects and ArchiveLinks HAVE been obsolete for 100+ days

@M-Zuber
Copy link
Contributor Author

M-Zuber commented Mar 29, 2016

😭
That's what I get for pushing my machines RAM too hard. The error in VS hadn't show up by the time the commit was pushed....

I'll wait for @shiftkey's response (may have to wait until after #build 😄 ) and follow his recommendation

@shiftkey
Copy link
Member

@ryangribble

what do you reckon to 🔥ing IApiConnection.GetRedirect(), eventhough it wasnt marked obsolete? Note that the concrete implementation, as well as multiple other places regarding Redirects and ArchiveLinks HAVE been obsolete for 100+ days

I'd be breaking my own rule here, and covering over an oversight of mine. Let's add the [Obsolete] to the interface and leave it alone for this release.

@ryangribble
Copy link
Contributor

so @M-Zuber just chasing up on this one, I take it you are good to rebase to latest master and also tackle the redirect methods according to @shiftkey's comments

@M-Zuber
Copy link
Contributor Author

M-Zuber commented Apr 11, 2016

I'm good to do the work, but on the road ATM.
Will attempt to get to it in the next day or two

@M-Zuber M-Zuber force-pushed the cleanup-obsolete-items branch from 1ca953d to 20d8dd2 Compare April 13, 2016 22:28
@M-Zuber
Copy link
Contributor Author

M-Zuber commented Apr 13, 2016

@shiftkey @ryangribble rebased and updated code with regards to .GetRedirect.
Would love if I could get some 👀 on the changes to make sure that the changes are correct.

@shiftkey
Copy link
Member

This all seems good to me. @ryangribble anything else to add here?

@ryangribble
Copy link
Contributor

All good! I love that feeling of cleaning things up 😀

@ryangribble
Copy link
Contributor

LGTM

@ryangribble ryangribble merged commit 172b1a6 into octokit:master Apr 14, 2016
@shiftkey shiftkey mentioned this pull request Jun 14, 2016
6 tasks
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.

3 participants