-
Notifications
You must be signed in to change notification settings - Fork 32
Implement HTML report generation and update report formats #123
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
Conversation
8ab4a23
to
357ee5a
Compare
Closes #50 - Allow Skunk to HTML output the results it generates - Added support for generating HTML reports alongside existing JSON format. - Introduced new classes for HTML report generation, including Overview, OverviewData, and FileData. - Updated the Reporter module to include HTML in the report generator formats. - Created a template for the HTML report with a responsive design. - Refactored JSON report generation to improve code organization and clarity.
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 know this is not ready yet, but I left some comments and I have one question
How will we generate the html report I tried
skunk -o skunk.html
but it did not work.
It generates a file call skunk.html
but it contains console output.
end | ||
|
||
# :reek:TooManyStatements | ||
def truncate |
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 have not been able to test how this would work, but I would be interested to see if this will work:
def truncate
# Get pwd and trucate it away from the file_path
pwd = Dir.pwd
file_path.sub!("#{pwd}/", '')
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.
this looks like a good idea:
I tested it out and it won't work when you are running skunk
outside of the project directory.
In the following example I was running skunk
outside of the project
folder:
> pwd
/Users/juan/code/ombu/oss/skunk
> skunk /Users/juan/code/ombu/project
pwd: /Users/juan/code/ombu/oss/skunk
file_path: /Users/juan/code/ombu/project/lib/blog_linter.rb
pwd: /Users/juan/code/ombu/oss/skunk
file_path: /Users/juan/code/ombu/project/lib/blog_linter.rb
Meaning this won't cut off the full path to convert it to a relative path.
Any idea how to deal with that?
665b094
to
74f28ab
Compare
Issue 1: Unused Variable Warning ✅ File: lib/skunk/generators/html/overview.rb:30 Problem: data = @DaTa was assigned but never used Fix: Removed the unused variable assignment Issue 2: Segmentation Fault ✅ File: lib/skunk/commands/status_sharer.rb:26 Problem: Net::HTTPOK === response causes segfault in Ruby 2.7 Fix: Changed to safer response.is_a?(Net::HTTPOK) comparison The Problem: The ERB template was using a local variable data that was assigned from @DaTa, but Ruby's static analysis couldn't detect that the variable was being used by the ERB template, causing: Ruby 2.6: NameError: undefined local variable or method 'data' Ruby 3.3: Same error Warning: "assigned but unused variable - data"
require "skunk/cli/application" | ||
require "skunk/config" | ||
|
||
Skunk::Config.formats = %i[json html] |
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 testing purposes you can use this script to run skunk against any path and setting any format.
Take into account that for now the console report is generated all the time no matter if you just select :json
or :html
or both.
> ./bin/console /path/to/project
or
> ./bin/console .
# | ||
# @return [Skunk::Command::StatusReporter] | ||
def execute | ||
RubyCritic::Config.formats = [:json] |
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.
This was blocking on purpose the generation of more format
reports, since now we will have the html
report, we want to remove this.
private | ||
|
||
# @return [Boolean] Check if sharing is enabled via environment variable | ||
def share_enabled? |
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.
Ruby 2.7 CI was running into some segmentation failures because the mocking of ENV["SHARE"]
.
AI says:
The Problem:
The test was using Object.stub_const(:ENV, env)
to stub the ENV constant, which can cause segmentation faults in Ruby 2.7 because ENV is a special system object that shouldn't be stubbed directly.
|
||
@status_message = | ||
if Net::HTTPOK === response | ||
if response.is_a?(Net::HTTPOK) |
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.
Ruby 2.6 CI was running into some segmentation failures:
AI says:
Segmentation Fault
File: lib/skunk/commands/status_sharer.rb:26
Problem: Net::HTTPOK === response causes segfault in Ruby 2.7
Fix: Changed to safer response.is_a?(Net::HTTPOK) comparison
# Before (segfault in Ruby 2.7):
if Net::HTTPOK === response
# After (safe for all Ruby versions):
if response.is_a?(Net::HTTPOK)
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 do not like this class; I'd love to have a simple solution to truncate the non-interested part of the file path... what Francois suggested sounded like a really good simple approach, but we won't cover scenarios where running skunk
out of the project path is exactly the scenario where we need the fix.
An alternative solution is to just leave the path as long as it would be, but that adds a lot of noise to the report.
Another is to use CSS to hide the beginning of the path when needed, but that is also not exactly precise, what if it hides all the way until the name of the file?
I do not know; suggestions are welcome.
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 we need to consider how skunk is used. I never used skunk outside of the project's path. Is that common?
Or better yet: does skunk even support that usage? If not, than I think you can implement the solution Francois suggested.
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.
The most significant change to this file is the FILE_NAME
change; it used to append the report into the report.json
file for the RubyCritic
report, which seems off, since Skunk
is an independent extension.
Other changes in this file are just to match the names we have in the SkunkData
class to make sure you all understand it is indeed the same data construction, and we will be unifying that logic into a module or class.
Generator.const_get("#{config_format.capitalize}Report") | ||
else | ||
require "skunk/generators/console_report" | ||
Generator::ConsoleReport |
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 of the next steps is to extract the console report that we currently generate into its own class, and this will allow us to stop generating it when we only request a specific format report.
but that code is tied and difficult to extract, though not impossible. (challenge accepted) 💪
|
||
**Supported formats:** | ||
- `:json` - JSON report (default) | ||
- `:html` - HTML report with visual charts and tables |
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.
- `:html` - HTML report with visual charts and tables | |
- `:html` - HTML report with visual tables |
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.
Are we to do anything about the new offenses? Because I saw you added exceptions. Maybe this file needs another update?
@JuanVqz Don't we need to bump the version as well? |
We don't the release happens in its own PR |
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.
Pull Request Overview
This PR implements HTML report generation for Skunk and adds a configuration system to control output formats. It introduces the ability to generate visual HTML reports alongside the existing JSON reports and provides a new Skunk::Config
class for programmatic format configuration.
- Adds HTML report generation with overview tables and visual styling
- Introduces
Skunk::Config
class for managing output formats programmatically - Updates JSON report generation to use Skunk's own location instead of RubyCritic's
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
lib/skunk/config.rb | New configuration system with format validation and management |
lib/skunk/generators/html_report.rb | Main HTML report generator class extending RubyCritic's base |
lib/skunk/generators/html/overview.rb | HTML overview page generator with ERB template rendering |
lib/skunk/generators/html/templates/skunk_overview.html.erb | HTML template with responsive design and modern styling |
lib/skunk/generators/html/skunk_data.rb | Data object for HTML report with calculated metrics |
lib/skunk/generators/html/file_data.rb | Individual file data wrapper for HTML display |
lib/skunk/generators/html/path_truncator.rb | Utility for truncating file paths to relevant project structure |
lib/skunk/generators/json/simple.rb | Updated JSON generator to be independent of RubyCritic |
lib/skunk/reporter.rb | Updated to use Skunk::Config instead of RubyCritic::Config |
test/lib/skunk/config_test.rb | Comprehensive tests for configuration functionality |
test/lib/skunk/generators/html/path_truncator_test.rb | Tests for path truncation utility |
test/lib/skunk/application_test.rb | Updated test stubs for sharing functionality |
lib/skunk/commands/status_sharer.rb | Refactored sharing logic with improved method extraction |
lib/skunk/commands/shareable.rb | Enhanced sharing interface with better method organization |
lib/skunk/commands/default.rb | Removed hardcoded RubyCritic format setting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
attr_reader :file, :skunk_score, :churn_times_cost, :churn, :cost, :coverage | ||
|
||
def initialize(module_data) | ||
@file = PathTruncator.truncate(module_data.pathname) |
Copilot
AI
Oct 9, 2025
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.
The PathTruncator class is not required in this file but is being used. Add require \"skunk/generators/html/path_truncator\"
at the top of the file.
Copilot uses AI. Check for mistakes.
FILE_NAME = "skunk_report.json" | ||
|
||
def render | ||
JSON.dump(data) |
Copilot
AI
Oct 9, 2025
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.
The JSON module is used but not required. Add require \"json\"
at the top of the file.
Copilot uses AI. Check for mistakes.
:root { | ||
/* Color Palette */ | ||
--primary-gradient-start:rgb(8, 58, 19); | ||
--primary-gradient-end: rgb(8, 98, 44);; |
Copilot
AI
Oct 9, 2025
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.
Remove the extra semicolon at the end of the CSS property.
--primary-gradient-end: rgb(8, 98, 44);; | |
--primary-gradient-end: rgb(8, 98, 44); |
Copilot uses AI. Check for mistakes.
Closes #50
Description:
formats
check theREADME.md
for more info.CHANGELOG.md
that links to this PR under the "main (unreleased)" heading.I will abide by the code of conduct.