Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Output customizations: REPORT_TITLE and OUTPUT_FILE #160
Changes from 3 commits
1693462
ae1df11
5c5cd84
8b227f0
98d9842
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@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). :)
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.
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
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.
Yeah I think that was aspirational. I don't think we actually wrote the code to support that.
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.
@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.
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.
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
- viamain()
,get_env_vars()
, and others.Yet again other ENV vars are read directly in
markdown_writer.py
.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.
Ideally we would consolidate all env variables being read from the get_env_vars() function.
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.
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.