ML icon indicating copy to clipboard operation
ML copied to clipboard

Prevent future inappropriate print statement usage with checkstyle-based build-time enforcement

Open Copilot opened this issue 5 months ago • 4 comments

Plan to Prevent Future Inappropriate Print Statement Usage

This PR implements a solution to prevent future uses of print statements where logging should be used instead, as requested in issue #331.

Plan:

  • [x] Analyze current codebase to identify existing print statement patterns
  • [x] Identify CLI-appropriate vs non-CLI directories for print statement usage
  • [x] Create a custom Maven plugin or RegEx-based checker to validate print statement usage
  • [x] Define allowed patterns for CLI code (under /driver/ packages)
  • [x] Configure the checker to fail the build when inappropriate print statements are found
  • [x] Add exclusions for legitimate print statement usage (CLI, formatters, main methods)
  • [x] Test the checker with both positive and negative test cases
  • [x] Document the new build rule and guidelines
  • [x] Address feedback: Fix formatting violations, switch to checkstyle, extend policy to test classes
  • [x] Fix whitespace errors: Applied spotless formatting to resolve all whitespace violations

COMPLETE IMPLEMENTATION - All Feedback Addressed

This PR successfully implements a professional checkstyle-based print statement policy enforcement that addresses all feedback including the latest whitespace formatting issues.

Latest Changes:

  1. ✅ Fixed Whitespace Errors: Applied mvn spotless:apply to fix whitespace formatting violations in:
    • docs/PRINT_STATEMENT_POLICY.md - Removed improper tab character
    • checkstyle.xml and checkstyle-suppressions.xml - Ensured proper XML formatting
    • All files now pass mvn spotless:check validation

Updated Implementation:

Build Integration:

  • Professional: Uses Maven Checkstyle plugin instead of bash script
  • Clean: All formatting violations resolved - passes mvn spotless:check
  • Functional: Still maintains skip.print.check property for development flexibility
  • Industry Standard: Professional integration with Maven ecosystem

Updated Policy Rules:

  • Allowed: Print statements in CLI driver classes (/driver/ packages)
  • Allowed: Print statements in main methods of utility/demo classes
  • Prohibited: Print statements in core library code, analysis engines, test classes

Technical Implementation:

  • checkstyle.xml - Main checkstyle configuration with RegexpSinglelineJava rule
  • checkstyle-suppressions.xml - Suppression rules for allowed cases
  • All files properly formatted and pass spotless validation
  • Removed old bash scripts (cleaner approach)

Demo Results:

# Formatting check - now passes
$ mvn spotless:check
[INFO] BUILD SUCCESS

# Checkstyle enforcement - still fails appropriately  
$ mvn validate
[ERROR] There are 17 errors reported by Checkstyle 10.20.1

# Skip functionality still works
$ mvn validate -Dskip.print.check=true
[INFO] BUILD SUCCESS (proceeds to next phase)

Detection Coverage: Detects violations in all Java files including test classes, with clean formatting and precise line-by-line reporting via checkstyle.

All Feedback Points Addressed:

  • Formatting violations: Fixed with mvn spotless:apply
  • Checkstyle adoption: Professional Maven Checkstyle integration
  • Test class policy: Extended to require logging in test classes
  • Whitespace errors: All formatting violations resolved

🎯 Complete Solution: Professional, clean, and functional build-time enforcement that prevents future inappropriate print statement usage.

Fixes #333.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot avatar Sep 23 '25 19:09 Copilot

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 57.91%. Comparing base (ef22e49) to head (96993c2).

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #334      +/-   ##
============================================
+ Coverage     57.90%   57.91%   +0.01%     
  Complexity      624      624              
============================================
  Files           111      111              
  Lines          7671     7671              
  Branches        856      856              
============================================
+ Hits           4442     4443       +1     
+ Misses         3051     3050       -1     
  Partials        178      178              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Sep 23 '25 19:09 codecov[bot]

@copilot Three things:

  1. You have formatting violations.
  2. Would it be better to use checkstyle instead?
  3. Do our current test classes use logging? If so, perhaps it would be more appropriate to disallow print statements in test code as well.

khatchad avatar Sep 23 '25 20:09 khatchad

@copilot Three things:

  1. You have formatting violations. ...

Addressed all three points:

  1. Formatting violations: Fixed with mvn spotless:apply in commit a059fbb

  2. Checkstyle vs bash script: You're right that checkstyle would be more professional and better integrated with Maven. I'll implement a checkstyle-based solution to replace the bash script.

  3. Test classes and logging: Good point - the test classes currently use print statements for debugging but don't use logging. Since they should follow the same best practices, I'll update the policy to disallow print statements in test code as well and suggest using logging there too.

I'll make these improvements now.

Copilot avatar Sep 23 '25 20:09 Copilot

@copilot Please fix the whitespace errors.

Fixed whitespace errors with mvn spotless:apply in commit 6d7b0b6. All formatting violations resolved - the build now passes mvn spotless:check validation.

Copilot avatar Sep 23 '25 20:09 Copilot