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

Output customizations: REPORT_TITLE and OUTPUT_FILE #160

Closed
wants to merge 5 commits into from

Conversation

spier
Copy link
Contributor

@spier spier commented Oct 29, 2023

Proposed Changes

Contributes to #156.

My plan is to implement customizability for both REPORT_TITLE and OUTPUT_FILE in this PR.

Status:

  • REPORT_TITLE - First working implementation of a customizable REPORT_TITLE
  • OUTPUT_FILE - Not implemented yet

I am sharing this PR early, to get feedback about preferred implementation and testability

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

@spier
Copy link
Contributor Author

spier commented Oct 29, 2023

@zkoppert I am adding some questions in inline comments, where I wasn't sure how to best implement things.

Copy link
Contributor Author

@spier spier left a comment

Choose a reason for hiding this comment

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

While writing the inline comments I think i convinced myself that I should implement the default value for the REPORT_TITLE at a different place. Looking forward to feedback though :)

issue_metrics.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
issue_metrics.py Outdated Show resolved Hide resolved
markdown_writer.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
test_markdown_writer.py Outdated Show resolved Hide resolved
num_issues_opened=num_issues_opened,
num_issues_closed=num_issues_closed,
labels=["bug"],
report_title="My Custom Report Title",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zkoppert I applied the suggested changes. That means moving the default value for the report title into the write_to_markdown() signature. It does lead to a better code structure overall, I think.

However one issue remains in this area:
We feed in the custom title as a parameter here.
Therefore we are not actually testing if it is passed through from the REPORT_TITLE env variable correctly.

Btw the same issue exists in the testing logic for the hide_label_metrics in row 305 above.
There I am not sure either if this test really covers what it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think overall I am a bit unclear about the intended logic of using ENV vars in the code.

Some ENV vars are read in various functions in issue_metrics.py - via main(), get_env_vars(), and others.
Yet again other ENV vars are read directly in markdown_writer.py.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would consolidate all env variables being read from the get_env_vars() function.

Copy link
Member

Choose a reason for hiding this comment

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

For this PR it would be good to follow that pattern. We can open a separate PR to get everything using the proper get_env_vars() function.

@@ -12,8 +12,7 @@
average_time_to_close: timedelta,
average_time_to_answer: timedelta,
num_issues_opened: int,
num_issues_closed: int,
file: file object = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zkoppert I found these traces in the documentation.

Apparently at some point in the past, a file object was passed to the write_to_markdown().
Do you happen to know anything about this?

As I am looking to add the feature to set a custom OUTPUT_FILE, I don't want to go down an implementation path that might have been already tried in the past (and did not work). :)

Copy link
Contributor Author

@spier spier Oct 31, 2023

Choose a reason for hiding this comment

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

Adding a link to the commit where I removed that unused bit of documentation. GitHub did not give me a straightforward way to get there from my comment above.
5c5cd84

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think that was aspirational. I don't think we actually wrote the code to support that.

@spier
Copy link
Contributor Author

spier commented Nov 7, 2023

@zkoppert just had a somewhat random idea:

What if we would implement the same functionality but with fewer ENV variables?

Idea

We could make the GHA append to issue_metrics.md, rather than to create a new file each time.
Then the users could run the report generation steps in the order, in which we want the reports to appear in the final GitHub issue.

That way we would not have to introduce any new ENV variables.

However, this would likely be a breaking change, for anybody that is already creating combined reports today.

Alternative

OR, we add only a single variable COMBINE_REPORTS = True, which when set will do the appending as described above. This way we might not create a breaking change, if the default for COMBINE_REPORTS was False?

Not 100% clear about the pros/cons but figured I just share it for input :)

@zkoppert
Copy link
Member

zkoppert commented Dec 2, 2023

@zkoppert just had a somewhat random idea:

What if we would implement the same functionality but with fewer ENV variables?

Idea

We could make the GHA append to issue_metrics.md, rather than to create a new file each time. Then the users could run the report generation steps in the order, in which we want the reports to appear in the final GitHub issue.

That way we would not have to introduce any new ENV variables.

However, this would likely be a breaking change, for anybody that is already creating combined reports today.

Alternative

OR, we add only a single variable COMBINE_REPORTS = True, which when set will do the appending as described above. This way we might not create a breaking change, if the default for COMBINE_REPORTS was False?

Not 100% clear about the pros/cons but figured I just share it for input :)

Because the alternative is a breaking change, I prefer the route of COMBINE_REPORTS = True.

jmeridth added a commit that referenced this pull request Sep 19, 2024
Closes #160

Adds the ability for users to designate custom report title
and output files.

Defaults for each:

REPORT_TITLE: "Issue Metrics"
OUTPUT_FILE: "issue_metrics.md" if markdown and "issue_metrics.json" if json

Signed-off-by: jmeridth <[email protected]>
Co-authored-by: Sebastian Spier <[email protected]>
Co-authored-by: Zack Koppert <[email protected]>
jmeridth added a commit that referenced this pull request Sep 19, 2024
Closes #160

- [x] Adds the ability for users to designate custom report title and output files.
  - Defaults for each:
    - REPORT_TITLE: "Issue Metrics"
    - OUTPUT_FILE: "issue_metrics.md" if markdown and "issue_metrics.json" if json
- [x] Update `.env-example` and include changes there and in docs we forgot in #370

Signed-off-by: jmeridth <[email protected]>
Co-authored-by: Sebastian Spier <[email protected]>
Co-authored-by: Zack Koppert <[email protected]>
@jmeridth
Copy link
Member

This has been open for a while. Completing in #373 and adding @spier and @zkoppert as co-authors. Once that merges, this will close. There are a few notes in the thread above that may turn into new issues (i.e., COMBINED_REPORTS = True).

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.

3 participants