-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add support to edit and delete a project #1434
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
Conversation
4359f4c
to
5f1e46f
Compare
I have created a single pull request for both the changes since the changes were similar. Let me know if this needs to be broken into two parts. Thanks! 😇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction, just a few things to tidy up before merging. Thanks for your hard work!
headers, data = self._requester.requestJsonAndCheck( | ||
"DELETE", self.url, headers={"Accept": Consts.mediaTypeProjectsPreview} | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other delete methods usually return True and check the status returned by GitHub, could you do the same here? You can then self.assertTrrue()
in the test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the documentation and the result of the call. The call to the above URL, doesn't return a True or False. It just gets us None
.
I checked the other methods as well, none of them return any data.
Lines 68 to 75 in b7894ea
def delete(self): | |
""" | |
:calls: `DELETE /repos/:owner/:repo/git/refs/:ref <http://developer.github.com/v3/git/refs>`_ | |
:rtype: None | |
""" | |
headers, data = self._requester.requestJsonAndCheck("DELETE", self.url) | |
Lines 250 to 256 in b7894ea
def remove_protection(self): | |
""" | |
:calls: `DELETE /repos/:owner/:repo/branches/:branch/protection <https://developer.github.com/v3/repos/branches>`_ | |
""" | |
headers, data = self._requester.requestJsonAndCheck( | |
"DELETE", self.protection_url, | |
) |
Lines 154 to 162 in fc4eff3
def delete(self): | |
""" | |
:calls: `DELETE /user/migrations/:migration_id/archive <https://developer.github.com/v3/migrations/users>`_ | |
""" | |
headers, data = self._requester.requestJsonAndCheck( | |
"DELETE", | |
self.url + "/archive", | |
headers={"Accept": Consts.mediaTypeMigrationPreview}, | |
) |
Let me know if there is a way around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was confusing it with another method, sorry.
|
||
def testEditWithoutParameters(self): | ||
project = self.g.get_project(4101939) | ||
project.edit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert the name or something here to just to check at least one of the attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved. Added a check to test the project name.
|
||
def testDelete(self): | ||
project = self.g.get_project(4102095) | ||
project.delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above -- we should be asserting the function return type too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned here, #1434 (comment), the delete method doesn't return a boolean. Not sure if it would be possible without that.
Checked other tests as well.
Lines 54 to 55 in 37122cf
def testDelete(self): | |
self.reactions[0].delete() |
Lines 69 to 70 in 37122cf
def testDelete(self): | |
self.ref.delete() |
Lines 68 to 69 in 37122cf
def testDelete(self): | |
self.comment.delete() |
Implements edit and delete project part of #606 (comment)
Manual testing
Before editing the project

After editing the project

After deleting the project
