Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 23, 2025

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:

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

Copy link

codecov bot commented Sep 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ 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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@Copilot Copilot AI changed the title [WIP] Prevent future uses of print statements where logging should be used instead Prevent future inappropriate print statement usage with build-time enforcement Sep 23, 2025
Copilot finished work on behalf of khatchad September 23, 2025 19:59
@Copilot Copilot AI requested a review from khatchad September 23, 2025 19:59
@khatchad
Copy link
Collaborator

@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.

Copy link
Contributor Author

Copilot AI commented Sep 23, 2025

@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.

… and update policy to include test classes

Co-authored-by: khatchad <[email protected]>
@Copilot Copilot AI changed the title Prevent future inappropriate print statement usage with build-time enforcement Prevent future inappropriate print statement usage with checkstyle-based build-time enforcement Sep 23, 2025
Copilot finished work on behalf of khatchad September 23, 2025 20:18
Copy link
Collaborator

@khatchad khatchad left a comment

Choose a reason for hiding this comment

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

@copilot Please fix the whitespace errors.

Copy link
Contributor Author

Copilot AI commented Sep 23, 2025

@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 Copilot AI requested a review from khatchad September 23, 2025 20:48
Copilot finished work on behalf of khatchad September 23, 2025 20:48
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.

Prevent future uses of print statements where logging should be used instead
2 participants