Implemented Lock/Unlock Functionality for Issues #1185

prayankmathur · 2016-03-14T05:03:28Z

@shiftkey
I have added the unit and integration tests for issue #896 . however the build is not running successfully.

prayankmathur · 2016-03-14T05:11:36Z

The build passed the checks here i guess.
Take a look at the unit and integration tests.

ryangribble · 2016-03-14T07:06:10Z

Hi @prayankmathur , could I suggest that you reword the pull request title and description to contain a little bit more details about what it's about?
You certainly want to link to the issue #896 but also would be good to say a few words here about it so everyone doesnt have to click through to the issue to see what this PR is about.

eg something like this:

Title:

Implement Lock/Unlock functionality for Issues

Description:

This PR implements the locking and unlocking funcitonality for Issues
Fixes #896
Also includes Unit and Integration Tests

Also from a quick look, this pull request includes your changes for ther other PR about ItemState/ItemStateFilter etc, which makes it hard to review the diffs in isolation. Ideally you would either do the work on separate branches, or once one PR gets merged, you can merge master into your branch and we will then see the "true" differences in this PR

prayankmathur · 2016-03-14T08:52:03Z

@ryangribble

Changed the title. Sorry for the trouble. Actually i did branch out this issue, but it turned out that i branched it from my master branch which consisted of the previous bug(which i hadn't branched).
If you could take the trouble of going through the other PR first, then it could solve the issue.

prayankmathur · 2016-03-14T09:34:25Z

@ryangribble
@shiftkey
I found that this function was not correct.
So corrected that too in the same PR.
Hope that it is fine.

shiftkey · 2016-03-14T21:57:46Z

@prayankmathur given it's not related to this feature, a separate PR would be nice for a couple of reasons:

we can review it separate to this
given it's a simpler change, it's likely to get merged in quicker

I'll leave it up to you whether you'd like to extract that to a new branch.

prayankmathur · 2016-03-14T22:00:45Z

@shiftkey
I ll open a separate PR.

ryangribble · 2016-03-15T23:56:54Z

Just noting for reference that the status of this PR is

Waiting for Added enum ItemStateFilter to differentiate between search and list APIs #1140 to be merged and then have this PR's commits tidied up (due to this PR being built ontop of Added enum ItemStateFilter to differentiate between search and list APIs #1140)
Waiting on @prayankmathur to split out the test fix for UpdateIssue to another PR and remove the commit from this one

prayankmathur · 2016-03-17T06:32:59Z

@ryangribble
I was waiting for #1140 to be merged.
But now i have opened a separate PR for the UpdateMethod function correction.

shiftkey · 2016-03-24T20:19:12Z

@prayankmathur this looks like it contains work from #1140 (open) and #1196 (merged). Are you able to rebase the branch so that this only contains the work related to the locking and unlocking of issues?

prayankmathur · 2016-03-25T06:37:32Z

Octokit.Tests/Reactive/ObservableIssuesClientTests.cs

+            Assert.Throws<ArgumentNullException>(() => client.Update("owner", null, 42, new IssueUpdate()));
+            Assert.Throws<ArgumentException>(() => client.Update("owner", "", 42, new IssueUpdate()));
+            Assert.Throws<ArgumentNullException>(() => client.Update("owner", "name", 42, null));
+        }


@shiftkey
Why is this function showing changes.
I had opened a seperate PR to change it which has been merged.
The function has changed also here

Had to remove it to remove conflicts.

prayankmathur · 2016-03-28T09:49:56Z

@shiftkey
I have made the necessary changes. Please take a look.

shiftkey · 2016-03-28T17:16:04Z

Octokit.Tests.Integration/Clients/IssuesClientTests.cs

+         var issue = await _issuesClient.Create(_context.RepositoryOwner, _context.RepositoryName, newIssue);
+         var retrieved = await _issuesClient.Get(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
+
+         Assert.Equal(false, issue.Locked);


You can use Assert.False or Assert.True here to make these a bit more succinct.

prayankmathur · 2016-03-29T10:29:15Z

@shiftkey @ryangribble
Should i rename the reactive function also ?
Because then it's usage would be like

client.Lock(owner, name, 1);

Currently its like

client.LockIssue(owner, name, 1)

ryangribble · 2016-03-29T10:38:29Z

Yes you should rename it too. In integration or unit tests, you are directly referencing the IObserableIssuesClient which might be what is throwing you off here...

Remember that "regular" users of octokit will be accessing both the standard or observable clients, through the "proper" heierarchy

eg

var client = new ObservableGitHubClient( ... );

await client.Issue.Lock(owner, name, 1);

prayankmathur · 2016-03-29T10:44:14Z

@ryangribble
Yeah, forgot that 😁
will make the changes.

ryangribble · 2016-03-29T10:56:44Z

Also dont forget you've got merge conflicts so either merge master into your branch, or rebase your branch on master and force push

ryangribble · 2016-03-29T11:27:24Z

Octokit.Tests/Reactive/ObservableIssuesClientTests.cs

@@ -337,6 +337,56 @@ public void EnsuresArgumentsNotNull()
        }
    }

+    public class TheLockIssueMethod


I guess this now should be "TheLockMethod"

ryangribble · 2016-03-29T11:31:14Z

Looks like you're ontop of the test failures which is 👍

Ive highlighted a couple of REALLY minor consistency issues around naming unit tests (now the methods have been renamed)

prayankmathur · 2016-03-29T11:53:56Z

@ryangribble
The changes are made successfully. You can now finally take a look. 😀

----EDIT----
travis CI failed because of a system failure

shiftkey · 2016-03-29T19:39:28Z

Octokit.Tests.Integration/Clients/IssuesClientTests.cs

+        await _issuesClient.Lock(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
+        retrieved = await _issuesClient.Get(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
+        Assert.NotNull(retrieved);
+        Assert.True(issue.Locked);


We should be checking retrieved.Locked here, right?

shiftkey · 2016-03-29T20:02:23Z

@prayankmathur this is getting closer - just added some things that I found while trying to get the tests to run...

ryangribble · 2016-03-30T01:07:20Z

Regarding the actual implementation of Lock and Unlock methods (which are API PUT methods that return a 204 response on success) I suggest we should implement them similarly to OrganizationMembers.Publicize()

In other words, check for that 204 response and return a Task<bool> to indicate success

prayankmathur · 2016-03-30T08:47:41Z

@ryangribble
@shiftkey
Right now, I will try to implement the

Delete(Uri uri, object data, string accepts)

And then later on we can open up a new PR.
Is that Fine ?

And please review the implementation of the Delete method. I was somewhat confused on how to do that.

Also should I implement the Lock/Unlock methods as suggested by @ryangribble which would return
Task<bool>

…nd changed definition for Unlock accordingly

prayankmathur · 2016-04-03T19:01:24Z

@shiftkey
@ryangribble
Just a gentle reminder to go see the changes that i made in this PR.

ryangribble · 2016-04-04T10:09:32Z

Also should I implement the Lock/Unlock methods as suggested by @ryangribble which would return
Task

Yes please 😀 @shiftkey also gave this a 👍 so we are in agreement that when the API docs mention a 204 response as success... you can check for that and return Task<bool> accordingly. Link to existing example: OrganizationMembers.Publicize()

ryangribble · 2016-04-04T10:12:38Z

Octokit/Http/Connection.cs

+        /// <param name="data">The object to serialize as the body of the request</param>
+        /// <param name="accepts">Specifies accept response media type</param>
+        /// <returns>The returned <seealso cref="HttpStatusCode"/></returns>
+        public async Task<HttpStatusCode> Delete(Uri uri,object data, string accepts)


missing space in between uri and object

This will be handled by the next FormatCode call, so let's 👢 addressing this...

shiftkey · 2016-04-05T21:14:23Z

I'm happy with the integration test now passing, and the minor stuff remaining can be sorted out later.

Thanks @prayankmathur!

prayankmathur · 2016-04-06T05:23:09Z

@shiftkey
👍

prayankmathur changed the title ~~Issue896~~ Implement Lock/Unlock Functionality for Issues Mar 14, 2016

prayankmathur changed the title ~~Implement Lock/Unlock Functionality for Issues~~ Implemented Lock/Unlock Functionality for Issues Mar 14, 2016

ryangribble added feedback given labels Mar 15, 2016

shiftkey removed the feedback given label Mar 17, 2016

prayankmathur force-pushed the issue896 branch from c55667b to 6f5d31e Compare March 25, 2016 05:57

Implement Lock/Unlock Issue

171b3c5

prayankmathur force-pushed the issue896 branch from f439c77 to 171b3c5 Compare March 25, 2016 06:22

Fixes

2c167bf

prayankmathur reviewed Mar 25, 2016
View reviewed changes

shiftkey reviewed Mar 28, 2016
View reviewed changes

Changes in Accept headers as well as names of the functions

8ff574a

Resolved conflicts

63bd122

ryangribble reviewed Mar 29, 2016
View reviewed changes

Changed function names in unit tests

55f4e72

Some more changes in tests

5173ade

shiftkey reviewed Mar 29, 2016
View reviewed changes

prayankmathur added 2 commits March 30, 2016 14:30

Added implementation for delete that takes custom media type header a…

8e49f04

…nd changed definition for Unlock accordingly

Minor fixes

7324039

ryangribble reviewed Apr 4, 2016
View reviewed changes

ryangribble mentioned this pull request Apr 4, 2016

Migrations API support #1141

Merged

12 tasks

shiftkey merged commit e8df990 into octokit:master Apr 5, 2016

prayankmathur deleted the issue896 branch April 6, 2016 05:14

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Lock/Unlock Functionality for Issues #1185

Implemented Lock/Unlock Functionality for Issues #1185

prayankmathur commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

ryangribble commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

shiftkey commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

ryangribble commented Mar 15, 2016

prayankmathur commented Mar 17, 2016

shiftkey commented Mar 24, 2016

prayankmathur Mar 25, 2016

prayankmathur commented Mar 28, 2016

shiftkey Mar 28, 2016

prayankmathur commented Mar 29, 2016

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 29, 2016

ryangribble commented Mar 29, 2016

ryangribble Mar 29, 2016

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 29, 2016

shiftkey Mar 29, 2016

shiftkey commented Mar 29, 2016

ryangribble commented Mar 30, 2016

prayankmathur commented Mar 30, 2016

prayankmathur commented Apr 3, 2016

ryangribble commented Apr 4, 2016

ryangribble Apr 4, 2016

shiftkey Apr 5, 2016

shiftkey commented Apr 5, 2016

prayankmathur commented Apr 6, 2016

Implemented Lock/Unlock Functionality for Issues #1185

Implemented Lock/Unlock Functionality for Issues #1185

Conversation

prayankmathur commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

ryangribble commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

shiftkey commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

ryangribble commented Mar 15, 2016

prayankmathur commented Mar 17, 2016

shiftkey commented Mar 24, 2016

prayankmathur Mar 25, 2016

Choose a reason for hiding this comment

prayankmathur commented Mar 28, 2016

shiftkey Mar 28, 2016

Choose a reason for hiding this comment

prayankmathur commented Mar 29, 2016

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 29, 2016

ryangribble commented Mar 29, 2016

ryangribble Mar 29, 2016

Choose a reason for hiding this comment

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 29, 2016

shiftkey Mar 29, 2016

Choose a reason for hiding this comment

shiftkey commented Mar 29, 2016

ryangribble commented Mar 30, 2016

prayankmathur commented Mar 30, 2016

prayankmathur commented Apr 3, 2016

ryangribble commented Apr 4, 2016

ryangribble Apr 4, 2016

Choose a reason for hiding this comment

shiftkey Apr 5, 2016

Choose a reason for hiding this comment

shiftkey commented Apr 5, 2016

prayankmathur commented Apr 6, 2016