Skip to content

Conversation

@scottbrown
Copy link
Owner

No description provided.

@scottbrown scottbrown requested a review from Copilot May 7, 2025 23:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for generating PDF reports, addressing issue #26. Key changes include:

  • Introducing PDF report generation in both overall and category contexts.
  • Updating tests in report_test.go to validate PDF report content and header.
  • Modifying command flags and report command logic to handle the new PDF format and adding the required gofpdf dependency.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

File Description
report_test.go Added tests for PDF report generation and refactored report output.
go.mod Added the gofpdf dependency to support PDF generation.
cmd/flag.go Updated flag help to include "pdf" as a valid report format.
cmd/cmd_report.go Modified report generation and output handling to accommodate PDF.

Comment on lines +423 to +424
if len(pdfReport.Content) < 4 || string(pdfReport.Content[:4]) != "%PDF" {
t.Error("Expected PDF content to start with '%PDF'")
Copy link

Copilot AI May 7, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider extracting the PDF header '%PDF' into a constant to improve code clarity and maintainability.

Suggested change
if len(pdfReport.Content) < 4 || string(pdfReport.Content[:4]) != "%PDF" {
t.Error("Expected PDF content to start with '%PDF'")
if len(pdfReport.Content) < 4 || string(pdfReport.Content[:4]) != PDFHeader {
t.Errorf("Expected PDF content to start with '%s'", PDFHeader)

Copilot uses AI. Check for mistakes.
Comment on lines +444 to +445
if len(categoryPDFReport.Content) < 4 || string(categoryPDFReport.Content[:4]) != "%PDF" {
t.Error("Expected PDF content to start with '%PDF'")
Copy link

Copilot AI May 7, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider extracting the PDF header '%PDF' into a constant to improve clarity in the category PDF tests as well.

Suggested change
if len(categoryPDFReport.Content) < 4 || string(categoryPDFReport.Content[:4]) != "%PDF" {
t.Error("Expected PDF content to start with '%PDF'")
if len(categoryPDFReport.Content) < 4 || string(categoryPDFReport.Content[:4]) != PDFHeader {
t.Errorf("Expected PDF content to start with '%s'", PDFHeader)

Copilot uses AI. Check for mistakes.
@scottbrown scottbrown merged commit f4fe5f9 into main May 7, 2025
7 checks passed
@scottbrown scottbrown deleted the issue-26 branch May 7, 2025 23:03
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.

2 participants