Skip to content

fix: remove mentor count from stats if mentors are disabled #519

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

Merged
merged 3 commits into from
May 14, 2025

Conversation

zkoppert
Copy link
Member

@zkoppert zkoppert commented May 8, 2025

Pull Request

Proposed Changes

This pull request introduces a new feature to enable tracking and reporting of mentor activity metrics in the issue metrics system. The changes primarily involve adding a new enable_mentor_count parameter across various functions and updating the logic to conditionally display mentor-related data in the output.

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
  • If publishing new data to the public (scorecards, security scan results, code quality results, live dashboards, etc.), please request review from @jeffrey-luszcz

Reviewer

  • Label as either fix, documentation, enhancement, infrastructure, maintenance, or breaking

Signed-off-by: Zack Koppert <zkoppert@github.com>
@zkoppert zkoppert self-assigned this May 8, 2025
@github-actions github-actions bot added the fix label May 8, 2025
@zkoppert zkoppert marked this pull request as ready for review May 14, 2025 16:53
@Copilot Copilot AI review requested due to automatic review settings May 14, 2025 16:53
@zkoppert zkoppert requested a review from a team as a code owner May 14, 2025 16:53
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new enable_mentor_count flag to conditionally include the mentor count in the markdown output.

  • Add enable_mentor_count parameter throughout writer functions and main invocation
  • Update tests to remove mentor count when flag is off and pass flag in one test
  • Conditionally write the mentor count row only when enabled

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
markdown_writer.py Added enable_mentor_count parameter and conditional mentor row
issue_metrics.py Passed enable_mentor_count from main into writer calls
test_markdown_writer.py Removed disabled-mentor-count expectations and set flag in a test
Comments suppressed due to low confidence (3)

test_markdown_writer.py:370

  • There’s no assertion verifying that the mentor count line actually appears when enable_mentor_count=True. Add something like assert '| Number of most active mentors |' in markdown_output to cover the enabled case.
enable_mentor_count=True,

issue_metrics.py:284

  • The CLI parser isn’t defining an --enable-mentor-count flag, so enable_mentor_count may always be unset or cause a NameError. Add an argument like parser.add_argument('--enable-mentor-count', action='store_true', help='Include mentor count in the report') before it's used.
enable_mentor_count=enable_mentor_count,

markdown_writer.py:103

  • The new enable_mentor_count parameter isn’t documented in the function docstring for write_to_markdown. Update the docstring to explain its purpose and default behavior.
enable_mentor_count=False,

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.

Actually using the boolean. Nice fix.

@zkoppert zkoppert merged commit 0ecfab7 into main May 14, 2025
32 checks passed
@zkoppert zkoppert deleted the zkoppert-hide-mentor-stats branch May 14, 2025 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants