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

bugfix: revise search algorithm for GitHub token #157

Merged
merged 5 commits into from
Nov 10, 2021

Conversation

prjemian
Copy link
Owner

@prjemian prjemian commented Nov 5, 2021

@prjemian prjemian added the task label Nov 5, 2021
@prjemian prjemian added this to the v1.0.0-release milestone Nov 5, 2021
@prjemian prjemian self-assigned this Nov 5, 2021
@prjemian
Copy link
Owner Author

prjemian commented Nov 5, 2021

@carterbox This is ready for review.

Copy link
Collaborator

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Wow, this function has gotten pretty complicated! 😆

What if the search order is in order of user-controlled to developer-controlled because this function is not part of the public API? Or the opposite ordering?

^ more user-y
GH_TOKEN
GITHUB_TOKEN
~/.config/__github_creds__.txt
~/.config/punx/__github_creds__.txt
creds_file_name This is the optional parameter; it is a path to any arbitrarily named text tile.
[punx source directory]/__github_creds__.txt
v more developer-y

punx/github_handler.py Outdated Show resolved Hide resolved
@prjemian
Copy link
Owner Author

prjemian commented Nov 5, 2021

I agree this is too complicated. Can we simplify to require only either of the two environment variables?

@carterbox
Copy link
Collaborator

Do you mean the search order would be:

GH_TOKEN
GITHUB_TOKEN

and the function would take no parameters? Sounds good to me.

@prjemian
Copy link
Owner Author

prjemian commented Nov 5, 2021 via email

@prjemian
Copy link
Owner Author

Now, still returns None if token not found but also issues a warning (that appears in the CI workflow):

================================================================== warnings summary ==================================================================
punx/tests/test_github_handler.py::test_connect_repo
punx/tests/test_github_handler.py::test_connected_GitHub_Repository_Reference
punx/tests/test_github_handler.py::test_Github_download_default
  /home/prjemian/Documents/projects/prjemian/punx/punx/github_handler.py:80: UserWarning: Did not find environment variables GH_TOKEN or GITHUB_TOKEN
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/warnings.html
=========================================================== 8 passed, 3 warnings in 0.87s ============================================================

@prjemian
Copy link
Owner Author

@carterbox - Docs will be modified if you agree on this approach.

Copy link
Collaborator

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

I agree with the approach overall. Comments about warning implementation below.

punx/github_handler.py Outdated Show resolved Hide resolved
punx/github_handler.py Outdated Show resolved Hide resolved
prjemian and others added 2 commits November 10, 2021 15:57
Co-authored-by: Daniel Ching <carterbox@users.noreply.github.com>
punx/github_handler.py Show resolved Hide resolved
@prjemian
Copy link
Owner Author

@carterbox Thanks for the review. Feel free to merge and delete the branch.

@carterbox carterbox merged commit 7fa9005 into main Nov 10, 2021
@carterbox carterbox deleted the 156-credentials-search branch November 10, 2021 23:52
prjemian added a commit that referenced this pull request Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit test for GitHub token fails when specific file name is provided
2 participants