-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: Adding 'Created_At' column, defaulting to false #460
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One code suggested to minimize duplication. Can you please add negative tests to test_config.py
and tests to test_markdown_writer.py
and test_json_writer.py
?
@@ -159,6 +161,9 @@ def get_per_issue_metrics( | |||
) | |||
elif issue.state == "open": # type: ignore | |||
num_issues_open += 1 | |||
if env_vars.hide_created_at is False: | |||
issue_with_metrics.created_at = issue.created_at | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not env_vars.hide_created_at: | |
issue_with_metrics.created_at = issue["createdAt"] |
we don't need the above in both the if and else. it can go once here. wdyt?
The issue["createdAt"]
also fixes your mypy linting issue.
Pulling your branch and looking into this. UPDATE: Fix below |
@@ -37,6 +37,7 @@ def __init__( | |||
time_in_draft=None, | |||
labels_metrics=None, | |||
mentor_activity=None, | |||
created_at=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update the docs above
The code suggestion fixes the linting issue. Since issue_with_metrics.created_at = issue["createdAt"] (.venv) ➜ issue-metrics git:(create_at) mypy --config-file=.github/linters/.mypy.ini *.py
Success: no issues found in 34 source files |
@Chocrates thank you for your contribution. Hopefully my comments above help you get past the linting issue and if you can look at the other suggestions. Cheers. |
Pull Request
Proposed Changes
We use this internally to pull metrics on open PR's and they would like to see the creation timestamp.
This PR adds the
created_at
column, defaulting to false with theHIDE_CREATED_AT
environment variable.Readiness Checklist
Author/Contributor
make lint
and fix any issues that you have introducedmake test
and ensure you have test coverage for the lines you are introducing@jeffrey-luszcz
Reviewer
fix
,documentation
,enhancement
,infrastructure
,maintenance
, orbreaking