Implemented IUserEmailsClient GetAll and Add methods #323

pmacn · 2014-01-23T18:27:12Z

Did not implement the Delete method as it is a DELETE that sends a request body.
http://developer.github.com/v3/users/emails/#delete-email-addresses

Not unheard of but still rather unorthodox especially since the body identified the resources to delete. So maybe it's time to start a lobbying group for that as well? If it's not something that's likely to change I'll go ahead and add the required infrastructure methods to handle deletes with a body, but would at least want to check first.

cc @pengwynn
Because you seem to be one of the people to call on for all things GitHub API

pmacn · 2014-01-23T18:35:28Z

Also looks as if this was already implemented in a different pull request.
Should have checked and not just put my trust in the list :)

shiftkey · 2014-01-23T18:38:25Z

@pmacn I really need to go back and revive those PRs that are languishing there, unloved...

pmacn · 2014-01-23T18:47:04Z

I'll update this to just add the Add method, however the current implementation has it sitting on the UsersClient and I created a separate UserEmailsClient for it.
Do we want to keep them on the UsersClient or should it be on a separate client as the API structure would suggest?

methods plus unit tests.

pmacn · 2014-01-23T20:24:59Z

I really need to go back and revive those PRs that are languishing there, unloved...

Their cries keep me up at night!
If it helps at all I can have a look at some of the ones that aren't mine and at least give a 👍 or 👎 (I could possibly even be nice and give suggestions/comments rather than just a thumbs down)

shiftkey · 2014-01-30T00:28:10Z

Octokit.Tests.Integration/Clients/UsersClientTests.cs

@@ -99,22 +99,4 @@ public class TheUpdateMethod
            Assert.Equal(HttpStatusCode.Unauthorized, e.StatusCode);
        }
    }
-
-    public class TheGetEmailsMethod


Plans to bring back this integration test?

Yes, there was a reason for this being out and I thought I put a comment in about it. I was obviously mistaken.
What I wanted to do was to try and replace it with something that would pass for everyone. Maybe simply check that it returns an email at all?
I guess another option would just be to have anyone who wants to run these change the email on their test account?
(Not sure if that's doable and it might leave a lot of test accounts with forgotten and un-retrievable passwords? :)

Maybe simply check that it returns an email at all?

👍 Let's start simple and go from there

not sure how I missed that before :-/

pmacn · 2014-02-02T00:07:10Z

Sorry about the delay haven't been able to get around to it until now.

shiftkey · 2014-02-02T22:35:55Z

Octokit/Clients/IUserEmailsClient.cs

+        /// <summary>
+        /// Gets all email addresses for the authenticated user.
+        /// </summary>
+        /// <returns></returns>


Finish up the returns elements here and I think this is ready to 🚢

…user_emails_client

and EmailAddress

EmailAddress.DebuggerDisplay

default implementation. Still needs delete method which is skipped because it's a HTTP DELETE. There's currently nothing in the ApiConnection to handle this scenario and it's a rather unorthodox thing to support. delete.

This only tests the GetAll method at this point. Would like to have Delete implemented before testing Add so that we can do cleanup.

pmacn · 2014-02-08T20:08:58Z

I think this has everything in it now. Except for the Delete method not being implemented because of the issues mentioned above.

pmacn · 2014-02-08T20:32:58Z

I'm receiving the following two errors when trying to run build.cmd all of a sudden

Running build failed.
Error:
System.Exception: Start of process C:\Windows\Microsoft.NET\Framework\v4.0.30319 failed. Access is denied
   at Fake.ProcessHelper.ExecProcessWithLambdas@68-16.Invoke(String message)
   at Fake.ProcessHelper.ExecProcessWithLambdas(FSharpFunc`2 configProcessStartInfoF, TimeSpan timeOut, Boolean silent, FSharpFunc`2 errorF, FSharpFunc`2 messageF)
   at Fake.MSBuildHelper.build(FSharpFunc`2 setParams, String project)
   at Fake.MSBuildHelper.MSBuildWithProjectProperties@271.Invoke(String project)
   at Microsoft.FSharp.Primitives.Basics.List.iter[T](FSharpFunc`2 f, FSharpList`1 x)
   at Fake.MSBuildHelper.MSBuildWithProjectProperties(String outputPath, String targets, FSharpFunc`2 properties, IEnumerable`1 projects)
   at Fake.MSBuildHelper.MSBuild@281-1.Invoke(IEnumerable`1 projects)
   at FSI_0001.clo@60-5.Invoke(Unit _arg5) in E:\Git\octokit.net.backup\build.fsx:line 61
   at Fake.TargetHelper.runTarget@290(String targetName)

and

  1) System.Exception: Start of process C:\Windows\Microsoft.NET\Framework\v4.0.30319 failed. Access is denied
   at Fake.ProcessHelper.ExecProcessWithLambdas@68-16.Invoke(String message)
   at Fake.ProcessHelper.ExecProcessWithLambdas(FSharpFunc`2 configProcessStartInfoF, TimeSpan timeOut, Boolean silent, FSharpFunc`2 errorF, FSharpFunc`2 messageF)
   at Fake.MSBuildHelper.build(FSharpFunc`2 setParams, String project)
   at Fake.MSBuildHelper.MSBuildWithProjectProperties@271.Invoke(String project)
   at Microsoft.FSharp.Primitives.Basics.List.iter[T](FSharpFunc`2 f, FSharpList`1 x)
   at Fake.MSBuildHelper.MSBuildWithProjectProperties(String outputPath, String targets, FSharpFunc`2 properties, IEnumerable`1 projects)
   at Fake.MSBuildHelper.MSBuild@281-1.Invoke(IEnumerable`1 projects)
   at FSI_0001.clo@60-5.Invoke(Unit _arg5) in E:\Git\octokit.net.backup\build.fsx:line 61
   at Fake.TargetHelper.runTarget@290(String targetName)

pmacn · 2014-02-08T20:35:57Z

Apparently the above two messages was because I had a custom environmental variable set called MSBUILD that pointed to that directory. Something I had set up for a different build.

Client and ObservableUserEmailsClient

shiftkey · 2014-02-10T00:18:55Z

Octokit.Tests.Integration/Reactive/ObservableUserEmailsClientTests.cs

+            };
+            var client = new ObservableUserEmailsClient(github);
+
+            var email = await client.GetAll();


Rx note: when you await on an observable you're doing the same thing as a .First() (which is now deprecated in Rx).

The test itself is just checking we get something back, which is fine, but just something to watch for. If you wanted all the results, you could await client.GetAll().ToArray()

which is now deprecated in Rx

Wasn't aware that it's been deprecated. I'll just make the test more Rxish in a separate PR.
But yes, my intention with the test was just to see that we got at least one EmailAddress back.

Implemented IUserEmailsClient GetAll and Add methods

Implemented UserEmailsClient GetAll and Add...

38aec16

methods plus unit tests.

shiftkey reviewed Jan 30, 2014
View reviewed changes

Peter MacNaughton added 3 commits February 1, 2014 16:54

Added IUserEmailsClient property to IUsersclient

0788c05

not sure how I missed that before :-/

Re-added integration test for getting email

871a6f5

Merge branch 'master' into user_emails_client

106e870

Peter MacNaughton added 2 commits February 1, 2014 17:30

Manual merge fail fixed

f51ee18

Merge branch 'master' into user_emails_client

aa13033

shiftkey reviewed Feb 2, 2014
View reviewed changes

Peter MacNaughton added 8 commits February 2, 2014 15:57

Added return values in documentation comments

bc3c433

Merge branch 'master' of https://github.com/octokit/octokit.net into …

172170a

…user_emails_client

Added DebuggerDisplay to EmailAddress model

1fffebc

Fixed documentation comments on UserEmailsClient

bfaccd0

and EmailAddress

Suppress Code Analaysis warning on

56cbb0c

EmailAddress.DebuggerDisplay

Adding IObservableUserEmailsClient and...

1332987

default implementation. Still needs delete method which is skipped because it's a HTTP DELETE. There's currently nothing in the ApiConnection to handle this scenario and it's a rather unorthodox thing to support. delete.

Unit tests for ObservableUserEmailsClient

a81a4d9

Integration tests for ObservableEmailsClient

3f6bb24

This only tests the GetAll method at this point. Would like to have Delete implemented before testing Add so that we can do cleanup.

FixProjects for IObservableUserEmailsClient

2421201

Fixing and adding unit tests for UserEmails...

2a9c7c7

Client and ObservableUserEmailsClient

shiftkey reviewed Feb 10, 2014
View reviewed changes

shiftkey added a commit that referenced this pull request Feb 10, 2014

Merge pull request #323 from pmacn/user_emails_client

70996fa

Implemented IUserEmailsClient GetAll and Add methods

shiftkey merged commit 70996fa into octokit:master Feb 10, 2014

shiftkey mentioned this pull request Feb 10, 2014

Implement User Emails - Delete operation #366

Closed

shiftkey mentioned this pull request Feb 10, 2014

Implement User Emails API #356

Closed

pmacn deleted the user_emails_client branch February 11, 2014 03:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented IUserEmailsClient GetAll and Add methods #323

Implemented IUserEmailsClient GetAll and Add methods #323

pmacn commented Jan 23, 2014

pmacn commented Jan 23, 2014

shiftkey commented Jan 23, 2014

pmacn commented Jan 23, 2014

pmacn commented Jan 23, 2014

shiftkey Jan 30, 2014

pmacn Jan 30, 2014

shiftkey Jan 30, 2014

pmacn commented Feb 2, 2014

shiftkey Feb 2, 2014

pmacn commented Feb 8, 2014

pmacn commented Feb 8, 2014

pmacn commented Feb 8, 2014

shiftkey Feb 10, 2014

pmacn Feb 10, 2014

Implemented IUserEmailsClient GetAll and Add methods #323

Implemented IUserEmailsClient GetAll and Add methods #323

Conversation

pmacn commented Jan 23, 2014

pmacn commented Jan 23, 2014

shiftkey commented Jan 23, 2014

pmacn commented Jan 23, 2014

pmacn commented Jan 23, 2014

shiftkey Jan 30, 2014

Choose a reason for hiding this comment

pmacn Jan 30, 2014

Choose a reason for hiding this comment

shiftkey Jan 30, 2014

Choose a reason for hiding this comment

pmacn commented Feb 2, 2014

shiftkey Feb 2, 2014

Choose a reason for hiding this comment

pmacn commented Feb 8, 2014

pmacn commented Feb 8, 2014

pmacn commented Feb 8, 2014

shiftkey Feb 10, 2014

Choose a reason for hiding this comment

pmacn Feb 10, 2014

Choose a reason for hiding this comment