Skip to content

Fix time calculation in the PRs script #359

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

Conversation

milancurcic
Copy link
Member

While preparing #357 I found a small issue in the PRs script which manifests only in December. This PR fixes it and adds a requirements.txt file to keep track of the script dependencies.

We should also have a short guide on making a newsletter, which is for another PR.

@milancurcic milancurcic added the bug Something isn't working label Jan 1, 2022
@milancurcic milancurcic requested a review from zmoon January 1, 2022 17:55
Copy link
Member

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

Oops, sorry about that, and thanks for fixing! We could do it with div-rem math and avoid dateutil, but that would be less clear.

@@ -3,6 +3,7 @@
Requires PyGithub.
Copy link
Member

Choose a reason for hiding this comment

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

could add python-dateutil to this note? or remove the line, as maybe it won't be necessary after the documentation you mentioned is added

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I've added requirements.txt which tracks dependencies, but we haven't yet documented this. So for now, I think it's good to mention it here, until it's documented elsewhere.

@@ -3,6 +3,7 @@
Requires PyGithub.
"""
import datetime
from dateutil.relativedelta import relativedelta
Copy link
Member

Choose a reason for hiding this comment

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

convention would be to put this after import github, to stress that it is not part of the std lib

Copy link
Member

Choose a reason for hiding this comment

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

I recommend to just run a formatter (black or similar) over the script.

Copy link
Member Author

Choose a reason for hiding this comment

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

Either sounds good. I haven't used formatters before.

Copy link
Member

@zmoon zmoon Jan 3, 2022

Choose a reason for hiding this comment

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

We could add a pre-commit config to do it automatically.

e.g.

# .pre-commit-config.yaml
repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: 'v4.1.0'
    hooks:
      - id: check-yaml
      - id: trailing-whitespace
        args: ['--markdown-linebreak-ext=md,markdown']
      - id: end-of-file-fixer

  - repo: https://github.com/asottile/reorder_python_imports
    rev: 'v2.6.0'
    hooks:
      - id: reorder-python-imports

  - repo: https://github.com/psf/black
    rev: '21.12b0'
    hooks:
      - id: black

☝️ Probably as separate PR since the eof check and trailing-whitespace would probably catch other files (if checking the whole repo).

Copy link
Member Author

Choose a reason for hiding this comment

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

I ran it through black. Let me know if there's anything else.

@zmoon
Copy link
Member

zmoon commented Mar 21, 2022

Maybe the requirements.txt should go inside the _scripts directory?

@milancurcic
Copy link
Member Author

OK, done, let me know if it's good to go.

@milancurcic milancurcic merged commit 886fcf9 into fortran-lang:master Apr 1, 2022
@milancurcic milancurcic deleted the fix-time-in-prs-script-2 branch April 1, 2022 21:20
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.

3 participants