From 217666bc28ebe58a42b8ecbe023624f95fe4ae4a Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Mon, 15 Apr 2024 17:06:18 -0700 Subject: [PATCH 1/7] fix: Performance fix to only measure statistics which are not hidden fixes #177 Signed-off-by: Zack Koppert --- issue_metrics.py | 102 +++++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 44 deletions(-) diff --git a/issue_metrics.py b/issue_metrics.py index f5c750a..628f7f3 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -26,7 +26,7 @@ import github3 from classes import IssueWithMetrics -from config import get_env_vars +from config import EnvVars, get_env_vars from discussions import get_discussions from json_writer import write_to_json from labels import get_label_metrics, get_stats_time_in_labels @@ -127,6 +127,7 @@ def get_per_issue_metrics( ignore_users: Union[List[str], None] = None, max_comments_to_eval: int = 20, heavily_involved: int = 3, + env_vars: EnvVars = None, ) -> tuple[List, int, int]: """ Calculate the metrics for each issue/pr/discussion in a list provided. @@ -138,6 +139,7 @@ def get_per_issue_metrics( Defaults to False. labels (List[str]): A list of labels to measure time spent in. Defaults to empty list. ignore_users (List[str]): A list of users to ignore when calculating metrics. + env_vars (EnvVars): The environment variables for the script. Returns: tuple[List[IssueWithMetrics], int, int]: A tuple containing a @@ -161,24 +163,30 @@ def get_per_issue_metrics( None, None, ) - issue_with_metrics.time_to_first_response = measure_time_to_first_response( - None, issue, ignore_users - ) - issue_with_metrics.mentor_activity = count_comments_per_user( - None, - issue, - ignore_users, - None, - None, - max_comments_to_eval, - heavily_involved, - ) - issue_with_metrics.time_to_answer = measure_time_to_answer(issue) - if issue["closedAt"]: - issue_with_metrics.time_to_close = measure_time_to_close(None, issue) - num_issues_closed += 1 - else: - num_issues_open += 1 + if env_vars.hide_time_to_first_response is False: + issue_with_metrics.time_to_first_response = ( + measure_time_to_first_response(None, issue, ignore_users) + ) + if env_vars.enable_mentor_count: + issue_with_metrics.mentor_activity = count_comments_per_user( + None, + issue, + ignore_users, + None, + None, + max_comments_to_eval, + heavily_involved, + ) + if env_vars.hide_time_to_answer is False: + issue_with_metrics.time_to_answer = measure_time_to_answer(issue) + if env_vars.hide_time_to_close is False: + if issue["closedAt"]: + issue_with_metrics.time_to_close = measure_time_to_close( + None, issue + ) + num_issues_closed += 1 + else: + num_issues_open += 1 else: issue_with_metrics = IssueWithMetrics( issue.title, # type: ignore @@ -196,32 +204,37 @@ def get_per_issue_metrics( pull_request = issue.issue.pull_request() # type: ignore ready_for_review_at = get_time_to_ready_for_review(issue, pull_request) - issue_with_metrics.time_to_first_response = measure_time_to_first_response( - issue, None, pull_request, ready_for_review_at, ignore_users - ) - issue_with_metrics.mentor_activity = count_comments_per_user( - issue, - None, - pull_request, - ready_for_review_at, - ignore_users, - max_comments_to_eval, - heavily_involved, - ) - if labels: - issue_with_metrics.label_metrics = get_label_metrics(issue, labels) - if issue.state == "closed": # type: ignore - if pull_request: - issue_with_metrics.time_to_close = measure_time_to_merge( - pull_request, ready_for_review_at + if env_vars.hide_time_to_first_response is False: + issue_with_metrics.time_to_first_response = ( + measure_time_to_first_response( + issue, None, pull_request, ready_for_review_at, ignore_users ) - else: - issue_with_metrics.time_to_close = measure_time_to_close( - issue, None - ) - num_issues_closed += 1 - elif issue.state == "open": # type: ignore - num_issues_open += 1 + ) + if env_vars.enable_mentor_count: + issue_with_metrics.mentor_activity = count_comments_per_user( + issue, + None, + pull_request, + ready_for_review_at, + ignore_users, + max_comments_to_eval, + heavily_involved, + ) + if labels and env_vars.hide_label_metrics is False: + issue_with_metrics.label_metrics = get_label_metrics(issue, labels) + if env_vars.hide_time_to_close is False: + if issue.state == "closed": # type: ignore + if pull_request: + issue_with_metrics.time_to_close = measure_time_to_merge( + pull_request, ready_for_review_at + ) + else: + issue_with_metrics.time_to_close = measure_time_to_close( + issue, None + ) + num_issues_closed += 1 + elif issue.state == "open": # type: ignore + num_issues_open += 1 issues_with_metrics.append(issue_with_metrics) return issues_with_metrics, num_issues_open, num_issues_closed @@ -324,6 +337,7 @@ def main(): ignore_users=ignore_users, max_comments_to_eval=max_comments_eval, heavily_involved=heavily_involved_cutoff, + env_vars=env_vars, ) stats_time_to_first_response = get_stats_time_to_first_response(issues_with_metrics) From 2b4cce2ba4aab2ffc5945d9505902c7f4ec24081 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 16 Apr 2024 10:53:48 -0700 Subject: [PATCH 2/7] fix: Set proper default. Co-authored-by: jmeridth Signed-off-by: Zack Koppert --- issue_metrics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/issue_metrics.py b/issue_metrics.py index 628f7f3..85c3b39 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -127,7 +127,7 @@ def get_per_issue_metrics( ignore_users: Union[List[str], None] = None, max_comments_to_eval: int = 20, heavily_involved: int = 3, - env_vars: EnvVars = None, + env_vars: EnvVars = get_env_vars(), ) -> tuple[List, int, int]: """ Calculate the metrics for each issue/pr/discussion in a list provided. From 0856d7db6d57887102385192c4aef112bca76e16 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 16 Apr 2024 14:26:16 -0700 Subject: [PATCH 3/7] set test bool when using it in tests Signed-off-by: Zack Koppert --- issue_metrics.py | 2 +- test_issue_metrics.py | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/issue_metrics.py b/issue_metrics.py index 85c3b39..15f58ee 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -122,12 +122,12 @@ def auth_to_github( def get_per_issue_metrics( issues: Union[List[dict], List[github3.search.IssueSearchResult]], # type: ignore + env_vars: EnvVars, discussions: bool = False, labels: Union[List[str], None] = None, ignore_users: Union[List[str], None] = None, max_comments_to_eval: int = 20, heavily_involved: int = 3, - env_vars: EnvVars = get_env_vars(), ) -> tuple[List, int, int]: """ Calculate the metrics for each issue/pr/discussion in a list provided. diff --git a/test_issue_metrics.py b/test_issue_metrics.py index d192157..582dc2e 100644 --- a/test_issue_metrics.py +++ b/test_issue_metrics.py @@ -108,8 +108,8 @@ def test_get_env_vars(self): """Test that the function correctly retrieves the environment variables.""" # Call the function and check the result - search_query = get_env_vars().search_query - gh_token = get_env_vars().gh_token + search_query = get_env_vars(test=True).search_query + gh_token = get_env_vars(test=True).gh_token gh_token_expected_result = "test_token" search_query_expected_result = "is:issue is:open repo:user/repo" self.assertEqual(gh_token, gh_token_expected_result) @@ -238,6 +238,10 @@ def test_main_no_issues_found( class TestGetPerIssueMetrics(unittest.TestCase): """Test suite for the get_per_issue_metrics function.""" + @patch.dict( + os.environ, + {"GH_TOKEN": "test_token", "SEARCH_QUERY": "is:issue is:open repo:user/repo"}, + ) def test_get_per_issue_metrics(self): """Test that the function correctly calculates the metrics for a list of GitHub issues.""" # Create mock data @@ -286,7 +290,7 @@ def test_get_per_issue_metrics(self): result_issues_with_metrics, result_num_issues_open, result_num_issues_closed, - ) = get_per_issue_metrics(issues) + ) = get_per_issue_metrics(issues, env_vars=get_env_vars(test=True)) expected_issues_with_metrics = [ IssueWithMetrics( "Issue 1", @@ -364,6 +368,10 @@ def setUp(self): "closedAt": "2023-01-07T00:00:00Z", } + @patch.dict( + os.environ, + {"GH_TOKEN": "test_token", "SEARCH_QUERY": "is:issue is:open repo:user/repo"}, + ) def test_get_per_issue_metrics_with_discussion(self): """ Test that the function correctly calculates @@ -371,7 +379,9 @@ def test_get_per_issue_metrics_with_discussion(self): """ issues = [self.issue1, self.issue2] - metrics = get_per_issue_metrics(issues, discussions=True) + metrics = get_per_issue_metrics( + issues, discussions=True, env_vars=get_env_vars(test=True) + ) # get_per_issue_metrics returns a tuple of # (issues_with_metrics, num_issues_open, num_issues_closed) From f76ff469d8d748b240b70f6e4cf4a9fdcdd3f6d0 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 16 Apr 2024 15:37:48 -0700 Subject: [PATCH 4/7] test: ensure via testing all test*.py files call get_env_vars() with test=True Signed-off-by: Zack Koppert --- .github/scripts/env_vars_check.sh | 22 ++++++++++++++++++++++ Makefile | 1 + 2 files changed, 23 insertions(+) create mode 100755 .github/scripts/env_vars_check.sh diff --git a/.github/scripts/env_vars_check.sh b/.github/scripts/env_vars_check.sh new file mode 100755 index 0000000..6b6eeb6 --- /dev/null +++ b/.github/scripts/env_vars_check.sh @@ -0,0 +1,22 @@ +#!/bin/bash + +# Find all test_*.py files +files=$(find . -name "test_*.py") +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' # No Color + +# Loop through each file +for file in $files; do + # Search for instances of get_env_vars() with no arguments + result=$(grep -n "get_env_vars()" "$file") + + # If any instances are found, print the file name and line number + if [ -n "$result" ]; then + echo "Found in $file:" + echo "$result" + echo -e "${RED}ERROR: get_env_vars() should always set test=True in test*.py files.${NC}" + exit 1 + fi +done +echo -e " ${GREEN}PASS:${NC} All test*.py files call get_env_vars() with test=True." \ No newline at end of file diff --git a/Makefile b/Makefile index 58d7479..c94fa87 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,7 @@ .PHONY: test test: pytest -v --cov=. --cov-config=.coveragerc --cov-fail-under=80 --cov-report term-missing + .github/scripts/env_vars_check.sh .PHONY: clean clean: From 33529fcba000b35c512daf5938cdc94790b9d502 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 16 Apr 2024 16:03:02 -0700 Subject: [PATCH 5/7] chore: add newline at EOF Signed-off-by: Zack Koppert --- .github/scripts/env_vars_check.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/env_vars_check.sh b/.github/scripts/env_vars_check.sh index 6b6eeb6..2d3fb01 100755 --- a/.github/scripts/env_vars_check.sh +++ b/.github/scripts/env_vars_check.sh @@ -19,4 +19,4 @@ for file in $files; do exit 1 fi done -echo -e " ${GREEN}PASS:${NC} All test*.py files call get_env_vars() with test=True." \ No newline at end of file +echo -e " ${GREEN}PASS:${NC} All test*.py files call get_env_vars() with test=True." From bef68c028f4235457684fbfc35e50c13a241e23b Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 16 Apr 2024 16:03:53 -0700 Subject: [PATCH 6/7] chore: more spaces means more better Signed-off-by: Zack Koppert --- .github/scripts/env_vars_check.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/scripts/env_vars_check.sh b/.github/scripts/env_vars_check.sh index 2d3fb01..779ce03 100755 --- a/.github/scripts/env_vars_check.sh +++ b/.github/scripts/env_vars_check.sh @@ -8,15 +8,15 @@ NC='\033[0m' # No Color # Loop through each file for file in $files; do - # Search for instances of get_env_vars() with no arguments - result=$(grep -n "get_env_vars()" "$file") + # Search for instances of get_env_vars() with no arguments + result=$(grep -n "get_env_vars()" "$file") - # If any instances are found, print the file name and line number - if [ -n "$result" ]; then - echo "Found in $file:" - echo "$result" - echo -e "${RED}ERROR: get_env_vars() should always set test=True in test*.py files.${NC}" - exit 1 - fi + # If any instances are found, print the file name and line number + if [ -n "$result" ]; then + echo "Found in $file:" + echo "$result" + echo -e "${RED}ERROR: get_env_vars() should always set test=True in test*.py files.${NC}" + exit 1 + fi done echo -e " ${GREEN}PASS:${NC} All test*.py files call get_env_vars() with test=True." From 2cdc7eac312a2fe236695842570f493f7b4f5c5d Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 16 Apr 2024 16:09:28 -0700 Subject: [PATCH 7/7] chore: use tabs in bash scripts Signed-off-by: Zack Koppert --- .github/scripts/env_vars_check.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/scripts/env_vars_check.sh b/.github/scripts/env_vars_check.sh index 779ce03..ecee7be 100755 --- a/.github/scripts/env_vars_check.sh +++ b/.github/scripts/env_vars_check.sh @@ -8,15 +8,15 @@ NC='\033[0m' # No Color # Loop through each file for file in $files; do - # Search for instances of get_env_vars() with no arguments - result=$(grep -n "get_env_vars()" "$file") + # Search for instances of get_env_vars() with no arguments + result=$(grep -n "get_env_vars()" "$file") - # If any instances are found, print the file name and line number - if [ -n "$result" ]; then - echo "Found in $file:" - echo "$result" - echo -e "${RED}ERROR: get_env_vars() should always set test=True in test*.py files.${NC}" - exit 1 - fi + # If any instances are found, print the file name and line number + if [ -n "$result" ]; then + echo "Found in $file:" + echo "$result" + echo -e "${RED}ERROR: get_env_vars() should always set test=True in test*.py files.${NC}" + exit 1 + fi done echo -e " ${GREEN}PASS:${NC} All test*.py files call get_env_vars() with test=True."