search

prjemian / punx

Python Utilities for NeXus HDF5 files
https://prjemian.github.io/punx
5 stars 7 forks source link

bugfix: revise search algorithm for GitHub token #157

Closed prjemian closed 2 years ago

prjemian commented 2 years ago
prjemian commented 2 years ago

@carterbox This is ready for review.

prjemian commented 2 years ago

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

carterbox commented 2 years ago

Do you mean the search order would be:

GH_TOKEN
GITHUB_TOKEN

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

prjemian commented 2 years ago

Yes

On Fri, Nov 5, 2021, 2:51 PM Daniel Ching @.***> wrote:

Do you mean the search order would be:

GH_TOKEN GITHUB_TOKEN

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

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/prjemian/punx/pull/157#issuecomment-962175319, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARMUMCNH2UJZR4NWRNYHX3UKQ7UFANCNFSM5HM5GK7Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

prjemian commented 2 years ago

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 commented 2 years ago

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

prjemian commented 2 years ago

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