X Tutup
The Wayback Machine - https://web.archive.org/web/20201125011843/https://github.com/TheAlgorithms/Python/pull/3949
Skip to content

/ Python

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

Jump to bottom

Fixes: #3944 authentication error #3949

Open
wants to merge 9 commits into
base: master
from
Open

Fixes: #3944 authentication error #3949

wants to merge 9 commits into from
+16 −9

Conversation

@lawric1
Copy link

@lawric1 lawric1 commented Nov 24, 2020 •
edited by dhruvmanila

Fixes: #3944

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.
@lawric1
fixes #3944 authentication error
Loading status checks…
7ab7847
@lawric1 lawric1 requested a review from cclauss as a code owner Nov 24, 2020
@algorithms-keeper
Copy link

@algorithms-keeper algorithms-keeper bot commented Nov 24, 2020

Pull Request Report

@lawric1 Hello! I'm a bot made to check all the pull request Python files. First of all, I want to say thank you for your time and interest in this project and for opening a pull request.

I have detected errors in some of the Python files submitted in this pull request. Please read through the report and make the necessary changes. You can take a look at the relevant links provided after the report.

What are node paths?

The report contain headings and a checklist where the items are paths to the class/function/parameter where the error is present. Node paths are double colon :: separated names and can be any of the following format:

  • Class path: [file_name]::[class_name]
  • Function path: [file_name]::[function_name]
  • Function parameter path: [file_name]::[function_name]::[parameter_name]
  • Method path: [file_name]::[class_name]::[function_name]
  • Method parameter path: [file_name]::[class_name]::[function_name]::[parameter_name]

Following functions require tests [doctest/unittest/pytest]:

  • web_programming/fetch_github_info.py::fetch_github_info

Relevant links:
@lawric1
Fixes: #3944 authentication error
Loading status checks…
6b11a5a
@dhruvmanila
dhruvmanila requested changes Nov 24, 2020
View changes
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Please read the GitHub REST API documentation and apply the requested changes.

Combine the module-level docstring into one and that should come first, before any of the imports.
web_programming/fetch_github_info.py Outdated Show resolved Hide resolved
web_programming/fetch_github_info.py Outdated Show resolved Hide resolved
web_programming/fetch_github_info.py Outdated Show resolved Hide resolved
web_programming/fetch_github_info.py Outdated Show resolved Hide resolved
lawric1 added 2 commits Nov 24, 2020
@lawric1
Fixed docstring failure in pre-commit, Fixed request.get params to Gi…
Loading status checks…
7b14334 
…tHub REST API standards
@lawric1
run black formatter
Loading status checks…
993ef9c
@dhruvmanila
dhruvmanila requested changes Nov 24, 2020
View changes
web_programming/fetch_github_info.py Outdated
Basic authentication using an API password is deprecated and will soon no longer work.
Visit https://developer.github.com/changes/2020-02-14-deprecating-password-auth
for more information around suggested workarounds and removal dates.
Comment on lines 5 to 7

This comment has been minimized.

Sign in to view
@dhruvmanila

dhruvmanila Nov 24, 2020
Member

As this is deprecated, please remove this.
web_programming/fetch_github_info.py Outdated


if __name__ == "__main__":
for key, value in fetch_github_info("<USER NAME>", "<PASSWORD>").items():
for key, value in fetch_github_info("<TOKEN>").items():

This comment has been minimized.

Sign in to view
@dhruvmanila

dhruvmanila Nov 24, 2020
Member

It would be better to add a global constant TOKEN with the default value an empty string "" and asking the user to provide their token.

Check whether the TOKEN constant is empty or not and raise a ValueError stating the fact that we need a token to get an authenticated user.
web_programming/fetch_github_info.py Outdated
import requests

_GITHUB_API = "https://api.github.com/user"


def fetch_github_info(auth_user: str, auth_pass: str) -> dict:
def fetch_github_info(auth_token: str) -> dict:

This comment has been minimized.

Sign in to view
@dhruvmanila

dhruvmanila Nov 24, 2020
Member

The return type hint needs to be more descriptive.

Use from typing import Dict and specify it as: Dict[type(key), type(value)]

As we probably don't know what GitHub is going to give us we should probably specify it as Dict[Any, Any]
web_programming/fetch_github_info.py Outdated
"""
Fetch GitHub info of a user using the requests module
"""
return requests.get(_GITHUB_API, auth=(auth_user, auth_pass)).json()
token = {"Authorization": "token {}".format(auth_token)}

This comment has been minimized.

Sign in to view
@dhruvmanila

dhruvmanila Nov 24, 2020
Member

Suggested change
token = {"Authorization": "token {}".format(auth_token)}
token = {"Authorization": f"token {auth_token}"}

Please use f-strings as mentioned in the Contributing guidelines.

Also, as suggested by GitHub, we should provide the Accept header value as well.
lawric1 added 3 commits Nov 24, 2020
@lawric1
Add USER_TOKEN constant and checks if empty, removes deprecated docst…
d640d40 
…ring
@lawric1
Add descriptive dict type hint, change headers format to f-string
cfb87dc
@lawric1
Add Accept header
Loading status checks…
ea5164e
lawric1 added 2 commits Nov 24, 2020
@lawric1
Fix pre-commit error
Loading status checks…
fa46152
@lawric1
Fix pre-commit error
Loading status checks…
76a6e56
@lawric1
Copy link
Author

@lawric1 lawric1 commented Nov 24, 2020

Thanks for all the feedback, this is my first time contributing and i'm learning a lot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhruvmanila dhruvmanila

@cclauss cclauss

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Require: Tests Status: awaiting changes Type: bugfix
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

web_programming/fetch_github_info.py err
2 participants
@lawric1 @dhruvmanila
You can’t perform that action at this time.
X Tutup