Skip to content

Add merged attribute to pulls, fix #335 #462

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.

Sign up for GitHub

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

Merged
sigmavirus24 merged 1 commit into from
Nov 6, 2015
Merged

Add merged attribute to pulls, fix #335 #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions github3/pulls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ def _update_attributes(self, pull):

#: Dictionary of _links. Changed in 1.0
self.links = pull.get('_links')
#: Boolean representing whether the pull request has been merged
self.merged = pull.get('merged')
#: datetime object representing when the pull was merged
self.merged_at = self._strptime(pull.get('merged_at'))
#: Whether the pull is deemed mergeable by GitHub
Expand Down Expand Up @@ -254,6 +256,9 @@ def is_merged(self):

:returns: bool
"""
if self.merged:
Copy link
Owner

@sigmavirus24 sigmavirus24 Nov 4, 2015

Choose a reason for hiding this comment

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

if it is not None. If it is False or True it came back from the API. You may not want to do this at all though because the cached information can be out of date.

Copy link
Contributor Author

@ekmartin ekmartin Nov 4, 2015

Choose a reason for hiding this comment

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

I was thinking that if a pull is merged, self.merged will always equal True in the future (even reverting the pull doesn't seem to change that), so there wouldn't be a point in checking the API. However, if self.merged is False, you'd probably want to be able to check the API to see if it has been merged yet?

The name of the method (is_merged) is a bit problematic though, as it sounds more like a getter for the merged attribute than something that would actually fire of an API request.

Copy link
Owner

@sigmavirus24 sigmavirus24 Nov 5, 2015

Choose a reason for hiding this comment

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

So the name is confusing if this is the first part of github3.py with which you interact with. Everything else does not retrieve a specific attribute and the merged attribute is a new addition to what the API returns. (Hence why it was missing and is_merged was not a bad name choice at the time.)

The pattern with the library is that attributes do not retrieve data for you or make API calls, but methods do. It's a logical distinction.

I would say someone can rely on pr.merged existing but they need to check that it's actually a boolean instead of just using is pr.merged. That said, I'm still conflicted about short-circuiting the method. I can't think of a place we do that elsewhere but I'm also in favor of saving people a request against their ratelimit.

Copy link
Contributor Author

@ekmartin ekmartin Nov 6, 2015

Choose a reason for hiding this comment

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

Yep, I understand. I don't mind removing the short-circuiting if you feel that's a better solution, just thought it would work alright here since as far as I know a merged pull request can never change status.

return self.merged

url = self._build_url('merge', base_url=self._api)
return self._boolean(self._get(url), 204, 404)

Expand Down
Loading