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

Top Ranking Issues #2267

Merged
merged 11 commits into from
Oct 19, 2024
Merged

Top Ranking Issues #2267

merged 11 commits into from
Oct 19, 2024

Conversation

kurt-rhee
Copy link
Contributor

  • [ x ] Closes Top Ranking Github Issues #2196 (Though we could keep this open to hold the top ranking issues)
  • [ x ] I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [ x ] Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@kurt-rhee
Copy link
Contributor Author

@AdamRJensen

The pull request here runs a script that ranks the pvlib python github issues by the number of thumbs up. It then edits #2196 to show the ordered list of issues.

A maintainer will need to add a github token as an environment variable and setup a github actions workflow that runs this script on a cron job. It might also be nice to pin issue 2196.

Other than that I think this is ready to go.

@kurt-rhee
Copy link
Contributor Author

I also think that perhaps I am using a different version of flake8. The linter in the PR says:

image

But from my perspective I am not seeing any unmatched brackets.

image

@AdamRJensen
Copy link
Member

  • I pinned the issue

In regards to the Token, then I don't think we should add one, but rather use the built in GitHub token: https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication

@echedey-ls
Copy link
Contributor

echedey-ls commented Oct 18, 2024

I also think that perhaps I am using a different version of flake8. The linter in the PR says:

image

But from my perspective I am not seeing any unmatched brackets.

image

You've probably figured it out already, but I think the problem are the string keys using double quotes, inside a double-quoted f-string. The following line should fix it, note single quotes:

f"{issue._rawData['reactions']['+1']} :thumbsup:"

Edit:
From a quick glance at the code, it looks super intuitive to maintain, I really like it!

Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

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

I feel like we should exclude the own issue from the list, before, in or after the for loop, wherever it is readable the most.

scripts/update_top_ranking_issues.py Outdated Show resolved Hide resolved
@kandersolar
Copy link
Member

The GH Actions job was set up to try to run on PRs so that we could test everything out here in this PR, but I'm not sure we're going to be able to get that working here. I think maybe the way forward is to get the script to a point where we like the looks of it, then merge, and then see whether it works as expected with the cron job. We can then do a follow-up PR with any fixes as needed.

Also @kurt-rhee, the YAML sets the environment variable GITHUB_ACCESS_TOKEN but the script is now looking for GITHUB_TOKEN. One should change to match the other; up to you which one :)

@kurt-rhee
Copy link
Contributor Author

@kandersolar woops! I thought that the token had to specifically be named GITHUB_TOKEN I've changed it back to match

@kurt-rhee
Copy link
Contributor Author

@echedey-ls thank you! And sorry it has been a while since I've worked in python 😳

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

I think we're ready for testing, so I'm going to merge this. @kurt-rhee, @AdamRJensen, and @echedey-ls, let's keep an eye on the pinned issue and the cron job logs and see what we think :)

If nothing else, we'll want to eventually edit the cron job to not run on PRs anymore, and to run less frequently than every 10 minutes.

@kandersolar kandersolar merged commit 2c61a64 into pvlib:main Oct 19, 2024
26 of 27 checks passed
@AdamRJensen
Copy link
Member

Awesome job @kurt-rhee!

Just checked the issue and it works like a charm!

@AdamRJensen AdamRJensen mentioned this pull request Oct 19, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Top Ranking Github Issues
4 participants