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

First stab at figuring out the number of very active mentors #206

Merged
merged 19 commits into from
Apr 11, 2024

Conversation

MaineC
Copy link
Contributor

@MaineC MaineC commented Feb 13, 2024

Relates to #114

This is a work in progress pull request. The goal is to extract the number of very active mentors in a project based on number of comments they left in issues and pull requests.

In order to avoid including endless discussion threads that often go in circles the idea is to limit the number of issue comments and pr comments taken into consideration.

This is also where I need input:

As in many other issue-metrics stats I used https://github3.readthedocs.io/en/1.0.1/issues.html#github3.issues.issue.Issue.comments in order to iterate over comments. In the documentation it says the iterator can limit the number of comments considered. In my test case though it seems to ignore this parameter. Likely I mis-understood the use of the iterator?

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 enhancement New feature or request label Mar 12, 2024

Open questions:
- should there be an option to limit this to certain users, e.g. core maintainers?
- should there be a limit to how many comments per PR we consider to avoid having
Copy link
Member

Choose a reason for hiding this comment

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

Yes, a limit makes sense to me. It may make sense to say that after 1 comment someone was involved with a PR, and after 3 comments they were heavily involved, and to stop counting after that. Those numbers are my personal thresholds in my mind but could be set to configurable numbers with reasonable defaults.

- should there be an option to limit this to certain users, e.g. core maintainers?
- should there be a limit to how many comments per PR we consider to avoid having
the statistic dominated by contested PRs?
- should this metric count consecutive comments coming from the same user as only
Copy link
Member

Choose a reason for hiding this comment

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

Great question. In this case I would also consider the length of the comment. If somebody has a lot to say and they break that up into several meaningful comments, I would see that as several contributions worth counting.

Copy link
Member

@zkoppert zkoppert left a comment

Choose a reason for hiding this comment

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

I see this as really valuable work! Thanks for working on it! ✨

For larger projects or organizations, it would be especially great to be able to see this data to be able to quickly find active projects that lack mentorship.

@zkoppert
Copy link
Member

next step for this one looks like working on fixing the tests.

@MaineC
Copy link
Contributor Author

MaineC commented Mar 26, 2024

I fixed the linter errors, ran the tests.

Still on my TODO list:

  • Option to enable/disable this check.
  • Output of results.

@MaineC
Copy link
Contributor Author

MaineC commented Mar 26, 2024

Output added to markdown and call added - and fixed the issues with the original design.

Still missing:

  • Output in json.
  • Additional configuration options for disabling this feature and configuring e.g. cutoff for who is considered an active mentor.

@MaineC
Copy link
Contributor Author

MaineC commented Mar 28, 2024

In the last commit I added mentor counting results to json output. In addition max number of comments per issue as well as the cutoff number for heavily involved mentors are now configurable.

Tests seem to run through locally, including the ones I added.

make lint left two warnings.

With that I think this is ready for getting a closer look and feedback.

Wish you Happy Easter (no, no Easter eggs hidden here ;) )

MaineC and others added 8 commits March 28, 2024 17:01
This adds the call to mentor counter and displays the results in markdown
including first tests for this functionality.
This adds two configuration options: One to enable mentor counting and one for
configuring how many comments a user needs to leave in discussions, PRs. and
issues to be counted as an active mentor.
This adds mentor counting output to json format. In addition this change makes
max number of comments to evaluate configurable as well as the cutoff for
heavily involved mentors.
@MaineC
Copy link
Contributor Author

MaineC commented Mar 28, 2024

Fixed merging errors.

There are two lint warnings still open that I can't figure out.

@zkoppert
Copy link
Member

I can dig into some of these linting errors. I'll set aside some time this coming week.

Signed-off-by: Zack Koppert <zkoppert@github.com>
Signed-off-by: Zack Koppert <zkoppert@github.com>
@zkoppert
Copy link
Member

zkoppert commented Apr 2, 2024

@MaineC I was able to correct the linting errors and override some unreasonable linting rules. Ready to click the ready for review button?

@MaineC MaineC marked this pull request as ready for review April 3, 2024 08:06
@MaineC MaineC requested a review from jmeridth as a code owner April 3, 2024 08:06
@MaineC
Copy link
Contributor Author

MaineC commented Apr 3, 2024

Clicked the button.

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.

Looks good. Just a couple asks. Thank you for your contribution.

config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
issue_metrics.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
test_issue_metrics.py Show resolved Hide resolved
test_most_active_mentors.py Outdated Show resolved Hide resolved
Co-authored-by: Jason Meridth <jmeridth@github.com>
@zkoppert
Copy link
Member

zkoppert commented Apr 5, 2024

@MaineC Feel free to review and integrate the change requests from @jmeridth.

Co-authored-by: Jason Meridth <jmeridth@github.com>
MaineC and others added 3 commits April 9, 2024 09:24
Remove merge residual
Remove lib only needed for testing.
Co-authored-by: Jason Meridth <jmeridth@github.com>
@MaineC
Copy link
Contributor Author

MaineC commented Apr 9, 2024

Finally back from Easter vacation/ family birthdays/ cold caught after that: Thanks for the feedback. I hope I caught all of the suggestions.

config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
jmeridth and others added 3 commits April 10, 2024 23:58
set type of `enable_mentor_count` to `bool`
change tests to handle boolean change of enable_mentor_count
@jmeridth jmeridth merged commit eb98ac5 into github:main Apr 11, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants