Adding new attributes to IssueEvent #857
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Travis tests have failedHey @allevin, 1st Buildpython setup.py test4th Buildpython setup.py test |
|
My testing has failed on Travis, but passes on my local machine. It seems from the Travis output that there is a mismatch in the authorization headers. I am using an auth token to login (I use 2 factor auth), and it records : However when testing is done in Travis, it seems to be using a username/password auth and is capturing: The problem seems to be in the fixAuthorizationHeader function in Framework.py, it looks to change the headers slightly for security, but the live data from travis does not match the recorded data when using a Token. Am I doing something wrong on my end? [Edit-2018/08/10] For this PR, it am just changing the recorded .txt file to use 'Basic login_and_password_removed'. However, I created a PR #861 that should correct this issue in the future once it is merged into the master branch. |
Travis tests have failedHey @allevin, 1st Buildpython setup.py test4th Buildpython setup.py test |
|
@sfdye I need someone with admin privileges to make the changes to this PR described above so I can write the additional tests for the new supported attributes. Most of them should be quite easy to implement (label on then off; assign a user then remove, Lock then unlock, etc). Once I get a few of these I will take another pass at integrating more tests. I expect this to take a few iterations to before we have all the event accounted for. |
|
Done the first three items. Can you check if it worked for you? If so, I will do the rest. |
sfdye
changed the title
allevin
changed the title
sfdye
changed the title
|
@sfdye Worked perfectly! Was able to generate the tests for the 4 new events. |
|
@allevin I just made a public project and linked to this repo. Did you get the remaining events now? |
…_columns_in_project and converted_note_to_issue
sfdye
reviewed
Aug 17, 2018
github/IssueEvent.py
Outdated
| @@ -101,6 +101,105 @@ def url(self): | |||
| self._completeIfNotSet(self._url) | |||
| return self._url.value | |||
|
|
|||
| # New Code | |||
There was a problem hiding this comment.
DOH! I missed that one ;-(
sfdye
reviewed
Aug 17, 2018
github/Issue.py
Outdated
| @@ -387,6 +387,9 @@ def get_events(self): | |||
| """ | |||
| :calls: `GET /repos/:owner/:repo/issues/:issue_number/events <http://developer.github.com/v3/issues/events>`_ | |||
| :rtype: :class:`github.PaginatedList.PaginatedList` of :class:`github.IssueEvent.IssueEvent` | |||
| Note: PaginatedList of events does not support per_page option, user must call | |||
There was a problem hiding this comment.
I am not sure this is the right place to put it
There was a problem hiding this comment.
Nor I, but I am unsure where else to put it. If its here, then it should show up in the online doc's telling the user of the issue.
sfdye
reviewed
Aug 17, 2018
github/IssueEvent.py
Outdated
| self._completeIfNotSet(self._dismissed_review) | ||
| return self._dismissed_review.value | ||
|
|
||
| # # lock_reason is documented in API, but not provided in response |
There was a problem hiding this comment.
You are right. Let me confirm this with Github support
|
LGTM. Let's wait what Github staff has to say about the missing attribute. |
|
@allevin Just got back from Github, it looks like there is a hidden custom header |
Travis tests have failedHey @allevin, 1st Buildpython setup.py test4th Buildpython setup.py test |
|
@sfdye Ok well the changes to IssueEvent and associated calls (Issue.get_events(), Repository.get_issues_event(), and Repository.get_issues_events()) with the custom header seem to work properly. However, the changes to Issue.py and Repository.py wont pass the tests (I assumed because of the custom header). I tried to record them, but am getting some odd errors... I tried an absolutely clean clone of PyGithub using the master branch, and the above command I am seeing a different failures on recording tests for Repository.py (again using PyGithub/master) |
sfdye
reviewed
Aug 24, 2018
github/Issue.py
Outdated
| @@ -395,7 +395,8 @@ def get_events(self): | |||
| github.IssueEvent.IssueEvent, | |||
| self._requester, | |||
| self.url + "/events", | |||
| None | |||
| None, | |||
| headers={'Accept': 'application/vnd.github.sailor-v-preview+json'} | |||
There was a problem hiding this comment.
Why do you have to add header here?
There was a problem hiding this comment.
BTW, all headers now have a designated place:
https://github.com/PyGithub/PyGithub/blob/master/github/Consts.py
There was a problem hiding this comment.
Issue.get_events() returns a IssueEvent object, we need the header here to ensure we get the lock_reason attribute.
|
@sfdye It was a bit of a battle, but I believe I got the custom header to support the |
|
Awesome! We can close this PR now. |
sfdye
approved these changes
Aug 24, 2018
|
@allevin Thank you for all the efforts put in this PR. Feel free to merge in :) |
candrikos
pushed a commit
to candrikos/PyGithub
that referenced
this pull request
Sep 25, 2020
See Issue PyGithub#855 The class [IssueEvent](https://github.com/PyGithub/PyGithub/blob/master/github/IssueEvent.py) is missing a large number of attributes documented in the [API](https://developer.github.com/v3/issues/events/). This is also commented about in PyGithub#653 to a degree 27 of the tests 27 known event types have tests. **Currently Tested using Issue PyGithub#30** - [x] subscribed - [x] assigned - [x] referenced - [x] closed - [x] labeled **Currently Tested using Issue/PR PyGithub#538** - [x] merged - [x] mentioned - [x] review_requested **Currently Tested using Issue/PR PyGithub#857** - [x] reopened - [x] unassigned - [x] unlabeled - [x] renamed - [x] base_ref_changed - [x] head_ref_deleted - [x] head_ref_restored - [x] milestoned - [x] demilestoned - [x] locked - [x] unlocked - [x] review_dismissed - [x] review_request_removed - [x] marked_as_duplicate - [x] unmarked_as_duplicate - [x] added_to_project - [x] moved_columns_in_project - [x] removed_from_project - [x] converted_note_to_issue - Note: this event is tied into Issue PyGithub#866 This PR is now ready to be merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Issue #855
The class IssueEvent is missing a large number of attributes documented in the API.
This is also commented about in #653 to a degree
27 of the tests 27 known event types have tests.
Currently Tested using Issue #30
Currently Tested using Issue/PR #538
Currently Tested using Issue/PR #857
This PR is now ready to be merged