Skip to content

Commit 0f74e43

Browse files
jacquev6jacquev6
committed
Review of PyGithub#189: pep8, copyrights, style, remarks
For remarks, run: git grep "PyGithub#189" They are only my first thoughts while reviewing this pull request, and should be reviewed themselves.
1 parent 1787765 commit 0f74e43

6 files changed

Lines changed: 32 additions & 21 deletions

File tree

‎github/Consts.py‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@
2121
# #
2222
################################################################################
2323

24-
# TODO: As of Thu Aug 21 22:40:13 (BJT) Chinese Standard Time 2013
24+
# #189: Line endings should be linux style
25+
26+
# TODO: As of Thu Aug 21 22:40:13 (BJT) Chinese Standard Time 2013
2527
# lots of consts in this project are explict
2628
# should realy round them up and reference them by consts
2729
# EDIT: well, maybe :-)
@@ -30,12 +32,12 @@
3032
# Request Header #
3133
# (Case sensitive) #
3234
################################################################################
33-
REQ_IF_NONE_MATCH = "If-None-Match"
34-
REQ_IF_MODIFIED_SINCE = "If-Modified-Since"
35+
REQ_IF_NONE_MATCH = "If-None-Match"
36+
REQ_IF_MODIFIED_SINCE = "If-Modified-Since"
3537

3638
################################################################################
3739
# Response Header #
3840
# (Lower Case) #
3941
################################################################################
40-
RES_ETAG = "etag"
41-
RES_LAST_MODIFED = "last-modified"
42+
RES_ETAG = "etag"
43+
RES_LAST_MODIFED = "last-modified"

‎github/GithubException.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
# #
55
# Copyright 2012 Vincent Jacques <vincent@vincent-jacques.net> #
66
# Copyright 2012 Zearin <zearin@gonk.net> #
7-
# Copyright 2013 Vincent Jacques <vincent@vincent-jacques.net> #
87
# Copyright 2013 AKFish <akfish@gmail.com> #
8+
# Copyright 2013 Vincent Jacques <vincent@vincent-jacques.net> #
99
# #
1010
# This file is part of PyGithub. http://jacquev6.github.com/PyGithub/ #
1111
# #
@@ -78,6 +78,7 @@ class RateLimitExceededException(GithubException):
7878
Exception raised when the rate limit is exceeded (when Github API replies with a 403 rate limit exceeded HTML status)
7979
"""
8080

81+
8182
class NotModifiedException(GithubException):
8283
"""
8384
Exception raised when conditional request is made to a resoure that has not changed

‎github/GithubObject.py‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,14 @@ def __init__(self, requester, headers, attributes, completed):
5757
self._requester = requester
5858
# Make sure headers are signed before any operations on attributes
5959
# Object creatation requires headers as parameter
60-
self._headers = headers;
60+
self._headers = headers
6161
self._initAttributes()
6262
self._storeAndUseAttributes(attributes)
6363

6464
# Ask requester to do some checking, for debug and test purpose
6565
# Since it's most handy to access and kinda all-knowing
66-
if (self.CHECK_AFTER_INIT_FLAG):
67-
requester.check_me(self);
66+
if self.CHECK_AFTER_INIT_FLAG:
67+
requester.check_me(self)
6868

6969
def _storeAndUseAttributes(self, attributes):
7070
self._useAttributes(attributes)
@@ -100,22 +100,21 @@ def _parseDatetime(s):
100100
else:
101101
return datetime.datetime.strptime(s, "%Y-%m-%dT%H:%M:%SZ")
102102

103-
def save(self, file_name):
103+
def save(self, file_name): # #189: Could we use file-like objects? It would be more "pythonic" than passing filenames.
104104
'''
105105
Save instance to a file
106-
107106
:param file_name: the full path of target file
108107
'''
109-
110108
with open(file_name, 'wb') as f:
111-
pickle.dump(self, f)
109+
pickle.dump(self, f) # #189: This will also save self._requester, and the login/password of the user. She might not appriciate.
110+
# #189: May be better to pickle only self._rawData and self._headers and restore the object with Github.create_from_raw_data
112111

113-
@classmethod
114-
def load(cls, file_name):
112+
@classmethod # #189: Could be a @staticmethod? The docstring would be simpler (no need to explain the type will be same as saved).
113+
def load(cls, file_name): # #189: Could we use file-like objects? It would be more "pythonic" than passing filenames.
115114
'''
116115
Load saved instance from file
117116
:param file_name: the full path to saved file
118-
:rtype: saved instance. The type of loaded instance remains its orginal one and will not be affected by from which derived class the method is called.
117+
:rtype: saved instance. The type of loaded instance remains its orginal one and will not be affected by from which derived class the method is called.
119118
'''
120119
with open(file_name, 'rb') as f:
121120
return pickle.load(f)
@@ -133,7 +132,6 @@ def last_modified(self):
133132
:type str
134133
'''
135134
return self._headers.get(Consts.RES_LAST_MODIFED)
136-
137135

138136
def update(self):
139137
'''
@@ -156,9 +154,10 @@ def update(self):
156154
self._storeAndUseAttributes(data)
157155
self.__completed = True
158156
return True
159-
except GithubException.NotModifiedException:
157+
except GithubException.NotModifiedException: # #189: Why raise and catch? Can't we just check?
160158
return False
161159

160+
162161
class NonCompletableGithubObject(GithubObject):
163162
def _completeIfNeeded(self):
164163
pass

‎github/Requester.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,8 @@ def __authenticate(self, url, requestHeaders, parameters):
290290
requestHeaders["Authorization"] = self.__authorizationHeader
291291

292292
def __conditional(self, requestHeaders, parameters):
293+
# #189: Why pass etag and last_modified by param "parameters"?
294+
# #189: May be better to add a specific param "headers" to methods requestFoobar?
293295
etag = parameters.get(Consts.REQ_IF_NONE_MATCH)
294296
last_modified = parameters.get(Consts.REQ_IF_MODIFIED_SINCE)
295297
if etag is not None:

‎github/tests/ConditionalRequestUpdate.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,17 @@
2121
# #
2222
################################################################################
2323

24+
# #189: Line endings should be linux style
25+
2426
import Framework
2527
import github
2628

29+
2730
class ConditionalRequestUpdate(Framework.TestCase):
2831
def setUp(self):
2932
Framework.TestCase.setUp(self)
30-
self.repo = self.g.get_repo("akfish/PyGithub")
33+
self.repo = self.g.get_repo("akfish/PyGithub")
34+
# #189: Let's separate this assert in its own test method, remove it from setUp.
3135
# Not updated
3236
self.assertFalse(self.repo.update(), msg="The repo is not changes. But update() != False")
3337

‎github/tests/_record_.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,20 @@
2828
import github.tests.AllTests
2929

3030

31+
# #189: This seems equivalent to "python -m github.tests ClassName.methodName --record"
32+
33+
3134
def main(argv):
3235
if len(argv) < 2:
3336
print "Run sepecified test in record mode."
3437
print "Usage:"
3538
print "_record_.py [module_name] [other_arg] ..."
3639
print " e.g. _record_.py AllTests"
3740
return
38-
41+
3942
github.tests.Framework.activateRecordMode()
4043
module_to_run = argv.pop(1)
41-
44+
4245
print "module: " + module_to_run
4346
print "argv: ",
4447
print argv

0 commit comments

Comments
 (0)