-
Notifications
You must be signed in to change notification settings - Fork 11
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
[FIX,TST] resolve refs when not run from a repo #35
Conversation
previous behavior used `_valid_git_reference_check` which assumes the target repo is cloned and up-to-date in the current directory Instead, always attempt to resolve refs with the GitHub API and only fallback on date parsing if it fails, avoiding the need to clone the repo.
repo has been migrated from choldgraf/github-activity to executablebooks org github-activity produces empty results when run on the old URL
Thanks for submitting your first pull request! You are awesome! 🤗 |
"{0} not found as a ref or valid date format".format( | ||
datetime_or_git_ref | ||
) | ||
) |
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.
As part of this refactoring, _valid_git_reference_check is no longer used. Can you delete its definition and the import of it as well?
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.
It is very helpful that you write except Exception as datetime_error:
even though datetime_error isn't used for readability, I'll remember this trick!
@choldgraf it is GitHub that did the automagic, since you merged that and this PR contained all commits part of that, github realized that this can be considered merged as well. |
I'll open a PR to resolve my comment |
I am so confused haha 😄 |
:D What happened was that you merged PR B which contained commit 1, 2, and 3. But this PR, PR A, contained commit 1, and 2. So, when you merged PR B, GitHub realized that it was equivalent to merging this first and then PR B, so it decided to consider this (PR A) merged. |
I had no idea that github was so smart about this! |
Get the tests passing again (related to #28 since tests aren't run on CI right now)
_validate_git_ref
assumes the target repo is cloned and up-to-date in the current directory. This is fixed by unconditionally attempting to resolve refs via GitHub API, and falling back on date parsing when that fails for any reason. The result is that repos no longer need to be cloned to collect activity between refs.