Skip to content
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

Code Cleanup - Github source #295

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Conversation

dat-a-man
Copy link
Collaborator

@dat-a-man dat-a-man commented Nov 29, 2023

All the rest of the API helper functions were defined two times, one in "init.py" and one in "helper.py" as well. This was the issue mentioned in issue #294. Now, all the helper functions are moved to "helper.py".

Tell us what you do here

  • implementing verified source (please link a relevant issue labelled as verified source)
  • fixing a bug (please link a relevant bug report)
  • improving, documenting, customizing existing source (please link an issue or describe below)
  • Code cleanup - some unused bits found in github source #294
  • anything else (please link an issue or describe below)

Relevant issue

issue # 294

More PR info

  • The rest API helper functions were deleted from "init.py" and updated in "helpers.py".
  • Earlier functions in "init.py" were used for the source, now, the same functions are called from "helpers.py".
  • The functions in "init.py" were used in the source, so those were given preference over the functions in "helpers.py"
  • Most of the functions were the same except "_run_graphql_query, _get_reactions_data"

…nit__,py" as well. That caused the confusion mentioned in issue #294. Now all the helper functions are moved to "helper.py".
@dat-a-man dat-a-man changed the title The rest API functions were defined two times in "helper.py" and "__i… Code Cleanup - Github source Nov 29, 2023
def get_rest_pages(access_token: str, query: str) -> Iterator[List[StrAny]]:
url = REST_API_BASE_URL + query
while True:
def _get_rest_pages(access_token: str, query: str) -> Iterator[List[StrAny]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

you import this function to another script, so imo prefix "_", which means "private function", doesn't make sense.
you can keep it like get_rest_pages.

same for get_reactions_data

Copy link
Collaborator Author

@dat-a-man dat-a-man Dec 2, 2023

Choose a reason for hiding this comment

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

sure, okay

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

Copy link
Collaborator

@AstrakhantsevaAA AstrakhantsevaAA left a comment

Choose a reason for hiding this comment

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

check unused imports (in init.py file), also run black command for auto-formatting

).json()
def _request() -> requests.Response:
r = requests.post(
"https://api.github.com/graphql",
Copy link
Collaborator

Choose a reason for hiding this comment

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

use GRAPHQL_API_BASE_URL instead of "https://api.github.com/graphql"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

print(f"got page {url}, requests left: " + r.headers["x-ratelimit-remaining"])
return r

url = "https://api.github.com" + query
Copy link
Collaborator

Choose a reason for hiding this comment

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

use REST_API_BASE_URL

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

)
return r

next_page_url = "https://api.github.com" + query
Copy link
Collaborator

Choose a reason for hiding this comment

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

use REST_API_BASE_URL instead of "https://api.github.com"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@AstrakhantsevaAA AstrakhantsevaAA merged commit 2156373 into master Dec 7, 2023
14 checks passed
@AstrakhantsevaAA AstrakhantsevaAA deleted the mn/github_code_cleanup branch December 7, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants