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

fix: Performance fix to only measure statistics which are not hidden #239

Merged
merged 7 commits into from
Apr 16, 2024

Conversation

zkoppert
Copy link
Member

@zkoppert zkoppert commented Apr 16, 2024

fixes #177

Pull Request

Please review cautiously as my confidence around these changes is low as I was in a rush. Happy to pair on a review.

Proposed Changes

This pull request primarily focuses on enhancing the functionality of the issue_metrics.py script by introducing a new EnvVars class and using it to control the execution of certain metrics calculations. The changes allow for more flexibility in terms of what metrics to calculate and when.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, or breaking

@zkoppert zkoppert added the bug Something isn't working label Apr 16, 2024
@zkoppert zkoppert requested a review from jmeridth as a code owner April 16, 2024 00:07
@zkoppert
Copy link
Member Author

@jmeridth Any ideas on how to handle a default argument for the EnvVars type?

issue_metrics.py:130: error: Incompatible default for argument "env_vars" (default has type "None", argument has type "EnvVars") [assignment]

@jmeridth
Copy link
Member

@jmeridth Any ideas on how to handle a default argument for the EnvVars type?

issue_metrics.py:130: error: Incompatible default for argument "env_vars" (default has type "None", argument has type "EnvVars") [assignment]

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.

You could do some funky stuff with Union[EnvVars,None] as the return type from get_env_vars() but then you have to test for None everywhere it is used. Above is "easier". wdyt?

zkoppert and others added 3 commits April 16, 2024 10:53
@zkoppert
Copy link
Member Author

@jmeridth ready for next round of review when you have time

Copy link
Member

@jmeridth jmeridth left a comment

Choose a reason for hiding this comment

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

LGTM just one nit.

.github/scripts/env_vars_check.sh Outdated Show resolved Hide resolved
@zkoppert zkoppert merged commit 8d98d18 into main Apr 16, 2024
22 checks passed
@zkoppert zkoppert deleted the only-measure-whats-reported branch April 16, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only measure and issue for the columns that are reported
2 participants