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

Add gist get support #225

Merged
merged 8 commits into from
Nov 21, 2013
Merged

Add gist get support #225

merged 8 commits into from
Nov 21, 2013

Conversation

SimonCropp
Copy link
Contributor

No description provided.

var retrieved = await this._gistsClient.Get(6305249);
Assert.NotNull(retrieved);
}

Copy link
Member

Choose a reason for hiding this comment

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

W H I T E S P A C E

Copy link
Contributor

Choose a reason for hiding this comment

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

And let's assert a little more than it's not null. 😄

@shiftkey
Copy link
Member

Looks like lots of the groundwork is in there for the rest of the Gist API - is that part of the plan or am I getting ahead of myself?

A few little whitespace issues, and that failing test, but it looks promising so far!

@SimonCropp
Copy link
Contributor Author

@shiftkey well I only needed Get. happy to continue with the rest in the background. it is mostly manual work.

the next thing I need is http://developer.github.com/v3/git/ is anyone working on this?

@shiftkey
Copy link
Member

@SimonCropp

happy to continue with the rest in the background. it is mostly manual work.

I'm cool with that. Stub out out the other methods with a NotImplementedException("You can submit a pull request for this to https://github.com/octokit/octokit.net/") exception so we don't forget them. cc @haacked

the next thing I need is http://developer.github.com/v3/git/ is anyone working on this?

Blobs and Trees are underway #188 and #194 - which I'll review again and probably merge in today for a 0.1.5 release early next week.

@SimonCropp
Copy link
Contributor Author

@shiftkey @haacked turns out secrets gists have a non-numeric id. so dont merge this yet

@SimonCropp
Copy link
Contributor Author

@shiftkey so can this be merged?

@shiftkey
Copy link
Member

In it's current form, the magic "Merge pull request" button is disabled.

I think the .csproj changes from master need to be merged in.

I'll also just run through the latest changes just in case I missed something.

[SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Get",
Justification = "Method makes a network request")]
Task<Gist> Get(string id);

Copy link
Member

Choose a reason for hiding this comment

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

a little less whitespace

@shiftkey
Copy link
Member

Oh, and run .\build.cmd FixProjects to sync the new files across the Mono* projects ✨

@SimonCropp
Copy link
Contributor Author

@shiftkey

.\build.cmd FixProjects
The system cannot find the path specified.
(Q)uit, (Enter) runs the build again

@shiftkey
Copy link
Member

.\build.cmd FixProjects
The system cannot find the path specified.
(Q)uit, (Enter) runs the build again

Are you running it from Powershell or cmd or something crazy I haven't tested on?

@SimonCropp
Copy link
Contributor Author

was running in CMD. but fails in powerhell as well

but i did notice another error flick up for a second. had to print screen it

2013-11-21 09_04_06-posh git octokit net master

@shiftkey
Copy link
Member

@SimonCropp do a git clean -xdf to clear up that file.

cc @forki to remind me to review that script and try-catch the way out

@SimonCropp
Copy link
Contributor Author

same error

seems u have a bunch of stuff that assume existence of tools\FAKE.Core\ which does not exist on my machine

@shiftkey
Copy link
Member

@SimonCropp I'm able to see this issue on master - digging a bit deeper into it

@SimonCropp
Copy link
Contributor Author

sooo. can we merge this one ;)

@haacked
Copy link
Contributor

haacked commented Nov 20, 2013

Build.cmd is prompting. I thought we removed that prompt. It shouldn't have any sort of user interaction like that.

@SimonCropp
Copy link
Contributor Author

@haacked @shiftkey happy to screen share if u guys want to troubleshoot on my machine

@forki
Copy link
Contributor

forki commented Nov 20, 2013

Actually I broke fake for one or two hours. Sorry about that. Maybe you got a broken version. Can you delete tools/fake and retry?

@shiftkey
Copy link
Member

@haacked

Build.cmd is prompting.

It still is. Let me fix it.

@SimonCropp your %LOCALAPPDATA%\NuGet\Cache\ is probably like this

screen shot 2013-11-20 at 3 38 33 pm

@forki I'm still seeing the 0KB file be downloaded to the local cache - are you able to publish a new release?

@SimonCropp
Copy link
Contributor Author

@shiftkey yep i have a 0byte file.

@forki
Copy link
Contributor

forki commented Nov 21, 2013

Still broken?? Wtf

@SimonCropp
Copy link
Contributor Author

@forki did u unlist 757? 756 looks good but i cant see 757. if u want to get nuget to fix it u prob need to push a fixed version with a higher number than the broken one

@shiftkey
Copy link
Member

Running FAKE.Core.2.1.758-alpha is now kicking off the build, but there's a different issue here with the generated SolutionInfo.cs file.

    internal static class AssemblyVersionInformation {
        internal const string Version = "0.1.5";
    }
}

That closing brace is causing some compile issues.

@forki
Copy link
Contributor

forki commented Nov 21, 2013

I'm really sorry. I hope it works now. Something broke during nuget push.

It teaches me a lesson to finally release FAKE 2.0 in order to detach the important projects from the prerelease channel.
I think everyone is already using 2.x so it should be ok

@SimonCropp
Copy link
Contributor Author

@shiftkey so the projects are now synced. anything else?

@shiftkey
Copy link
Member

@SimonCropp I'm still seeing the broken SolutionInfo.cs file in FAKE version 2.1.766.

@forki is there something I'm missing?

@forki
Copy link
Contributor

forki commented Nov 21, 2013

This is so embarrassing.

I reverted FAKE to a good version. Octokit now builds on my machine.

I promise to release FAKE 2 today and then we can move Octokit and other important projects on the stable release channel.

I'm really sorry that I broke it. But I will give my best to improve it.

@shiftkey
Copy link
Member

@forki that's fine - happens to the best of us. Let me know when it's 🆒 and we can try again

@forki
Copy link
Contributor

forki commented Nov 21, 2013

Thanks.

Please try again the "good" (old) version is already on nuget.

@shiftkey
Copy link
Member

@forki got it

@SimonCropp looks good, would be nice to test that some of the properties on the gist response - like Owner for example - is not null

Let's see if @haacked has any issues

@SimonCropp
Copy link
Contributor Author

@shiftkey well my intent was actually to do some serialization tests on a future PR. but I initially wanted to "make it work" then iterate on that

@haacked
Copy link
Contributor

haacked commented Nov 21, 2013

Fine with me

shiftkey added a commit that referenced this pull request Nov 21, 2013
@shiftkey shiftkey merged commit 074dce7 into octokit:master Nov 21, 2013
@SimonCropp
Copy link
Contributor Author

Merged and Closed = Win :)

tailgate

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.

4 participants