-
Notifications
You must be signed in to change notification settings - Fork 410
fix: support for Ghost GH user (for deleted users) #1249
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
base: main
Are you sure you want to change the base?
Conversation
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'm not sure that an integration test with betamax is necessary, but if you were to copy
github3.py/tests/integration/test_github.py
Line 762 in 08fe91c
def test_user(self): |
def test_not_found_user_returns_default_ghost(self):
"""Test the ability to retrieve a User."""
cassette_name = self.cassette_name("not_found_user_returns_default_ghost")
with self.recorder.use_cassette(cassette_name):
ghost = self.gh.user("sigmavirus124")
assert isinstance(ghost, github3.users.User)
try:
assert repr(ghost)
except UnicodeEncodeError:
self.fail(
"Regression caught. See PR #52. Names must be utf-8"
" encoded"
)
Then run the test and check-in the resulting JSON file.
src/github3/users.py
Outdated
json = { | ||
"login": "ghost", | ||
"id": 10137, | ||
"node_id": "MDQ6VXNlcjEwMTM3", | ||
"avatar_url": "https://avatars.githubusercontent.com/u/10137?v" | ||
"=4", | ||
"gravatar_id": "", | ||
"url": "https://api.github.com/users/ghost", | ||
"html_url": "https://github.com/ghost", | ||
"followers_url": "https://api.github.com/users/ghost/followers", | ||
"following_url": "https://api.github.com/users/ghost/following" | ||
"{/other_user}", | ||
"gists_url": "https://api.github.com/users/ghost/gists{/gist_id" | ||
"}", | ||
"starred_url": "https://api.github.com/users/ghost/starred{/own" | ||
"er}{/repo}", | ||
"subscriptions_url": "https://api.github.com/users/ghost/subscr" | ||
"iptions", | ||
"organizations_url": "https://api.github.com/users/ghost/orgs", | ||
"repos_url": "https://api.github.com/users/ghost/repos", | ||
"events_url": "https://api.github.com/users/ghost/events{/priva" | ||
"cy}", | ||
"received_events_url": "https://api.github.com/users/ghost/rece" | ||
"ived_events", | ||
"type": "User", | ||
"user_view_type": "public", | ||
"site_admin": False, | ||
"name": "Deleted user", | ||
"company": None, | ||
"blog": "", | ||
"location": "Nothing to see here, move along.", | ||
"email": None, | ||
"hireable": None, | ||
"bio": "Hi, I'm @ghost! I take the place of user accounts that " | ||
"have been deleted.\n:ghost:\n", | ||
"twitter_username": None, | ||
"public_repos": 0, | ||
"public_gists": 0, | ||
"followers": 11584, | ||
"following": 0, | ||
"created_at": "2008-05-13T06:14:25Z", | ||
"updated_at": "2018-04-10T17:22:33Z", | ||
} |
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'd rather pull this into a module constant, e.g,
_ghost_json: typing.Final[typing.Dict[str, typing.Any]] = {
# ..
}
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.
Check out 6c3b789 to see if I interpreted you correctly. Or did you mean in src/github3/__init__.py
?
tests/unit/test_users.py
Outdated
|
||
def setUp(self): | ||
"""Use None to create a ghost user.""" | ||
self.session = self.create_session_mock() |
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.
self.session = self.create_session_mock() | |
super().setup() |
There's also after_setup()
for overriding things:
github3.py/tests/unit/helper.py
Lines 192 to 194 in 08fe91c
def after_setup(self): | |
"""No-op method to avoid people having to override setUp.""" | |
pass |
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 took the after_setup
approach in 615e464
Also looks like PyCQA/doc8#162 still isn't fixed. |
The thing is... this only happens, as far as I can tell, in very corner cases with releases. For example,
thanks! I added If you think this is an overkill, we can remove it. |
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.
One last comment I'd request you address, otherwise this looks really great
Let me know if I should do something else. btw, thanks for your support so far |
Fixes #988
Alternative to #1161
Version Information
Please provide:
The version of Python you're using: 3.11
The version of pip you used to install github3.py: 25.2
The version of:
Minimum Reproducible Example
Exception information
What is exceptional about what you're seeing versus what you expected to see.
Deleted users break the lib. From the conversation #1161 (comment) , it seems that github3 should create a GhostUser when
github3.users.User
is called withNone
.Some things to consider: