-
Notifications
You must be signed in to change notification settings - Fork 14
Improve branch filtering and raise error on empty dataframes #162
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,7 +123,9 @@ | |
| "--branch", | ||
| "-b", | ||
| default=None, | ||
| help=("""The branch or reference name to filter pull requests by"""), | ||
| help=( | ||
| """Filter pull requests by their target branch. Only PRs merged into this branch will be included. """ | ||
| ), | ||
| ) | ||
| parser.add_argument( | ||
| "--all", | ||
|
|
@@ -223,31 +225,36 @@ def main(): | |
| ignored_contributors=args.ignore_contributor, | ||
| ) | ||
|
|
||
| if args.all: | ||
| md = generate_all_activity_md(args.target, **common_kwargs) | ||
| # Wrap in a try/except so we don't have an ugly stack trace if there's an error | ||
| try: | ||
| if args.all: | ||
| md = generate_all_activity_md(args.target, **common_kwargs) | ||
|
Comment on lines
+228
to
+231
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is all just tab-adding. The only difference is the try/except statement, so that if valueerrors happen down below we can display them nicely. |
||
|
|
||
| else: | ||
| md = generate_activity_md( | ||
| args.target, | ||
| since=args.since, | ||
| until=args.until, | ||
| heading_level=args.heading_level, | ||
| **common_kwargs, | ||
| ) | ||
| else: | ||
| md = generate_activity_md( | ||
| args.target, | ||
| since=args.since, | ||
| until=args.until, | ||
| heading_level=args.heading_level, | ||
| **common_kwargs, | ||
| ) | ||
|
|
||
| if not md: | ||
| return | ||
|
|
||
| if args.output: | ||
| output = os.path.abspath(args.output) | ||
| output_dir = os.path.dirname(output) | ||
| if not os.path.exists(output_dir): | ||
| os.makedirs(output_dir) | ||
| with open(args.output, "w") as ff: | ||
| ff.write(md) | ||
| print(f"Finished writing markdown to: {args.output}", file=sys.stderr) | ||
| else: | ||
| print(md) | ||
| if not md: | ||
| return | ||
|
|
||
| if args.output: | ||
| output = os.path.abspath(args.output) | ||
| output_dir = os.path.dirname(output) | ||
| if not os.path.exists(output_dir): | ||
| os.makedirs(output_dir) | ||
| with open(args.output, "w") as ff: | ||
| ff.write(md) | ||
| print(f"Finished writing markdown to: {args.output}", file=sys.stderr) | ||
| else: | ||
| print(md) | ||
| except ValueError as e: | ||
| print(f"Error: {e}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -462,8 +462,27 @@ def generate_activity_md( | |
| data = get_activity( | ||
| target, since=since, until=until, kind=kind, auth=auth, cache=False | ||
| ) | ||
|
|
||
| # Raise error if GitHub API returned no activity at all | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noting this could be causing a change of behavior in the jupyter releaser, as noticed in some workflows today: https://github.com/jtpio/jupyterlite-server-contents/actions/runs/20604073817/job/59175936047
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the releaser was likely not expecting this call to raise, wondering if this could be considered somewhat breaking? |
||
| # This happens when the repository has no issues/PRs in the date range | ||
| if data.empty: | ||
| return | ||
| raise ValueError( | ||
| f"No activity found for {org}/{repo} between {since} and {until}." | ||
| ) | ||
|
|
||
| # Filter the PRs by branch (or ref) if given | ||
| if branch is not None: | ||
| index_names = data[ | ||
| (data["kind"] == "pr") & (data["baseRefName"] != branch) | ||
| ].index | ||
| data.drop(index_names, inplace=True) | ||
|
|
||
| # Raise error if branch filter removed all data | ||
| # This happens when PRs exist but none targeted the specified branch | ||
| if data.empty: | ||
| raise ValueError( | ||
| f"Found activity, but none for the --branch target specified for {org}/{repo} on branch '{branch}' between {since} and {until}.\n" | ||
| ) | ||
|
|
||
| # Collect authors of comments on issues/prs that they didn't open for our attribution list | ||
| comment_response_cutoff = 6 # Comments on a single issue | ||
|
|
@@ -581,15 +600,6 @@ def filter_ignored(userlist): | |
| ].index.tolist() | ||
| all_contributors |= set(c for c in comment_contributors if isinstance(c, str)) | ||
|
|
||
| # Filter the PRs by branch (or ref) if given | ||
| if branch is not None: | ||
| index_names = data[ | ||
| (data["kind"] == "pr") & (data["baseRefName"] != branch) | ||
| ].index | ||
| data.drop(index_names, inplace=True) | ||
| if data.empty: | ||
| return | ||
|
|
||
| # Extract datetime strings from data attributes for pandas query | ||
| since_dt_str = data.since_dt_str # noqa: F841 | ||
| until_dt_str = data.until_dt_str # noqa: F841 | ||
|
|
||
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.
This is just documenting pre-existing functionality