Skip to content

Conversation

@italomatos
Copy link
Contributor

🔧 Fix Rubocop violations and modernize configuration

📋 Summary

This PR resolves 48+ Rubocop style violations across the codebase and modernizes the Rubocop configuration files for compatibility with newer versions.

🚨 Problem Identified

When attempting to run Rubocop on the current codebase, the following compatibility error occurs:

httplog git:(master) rubocop -A lib/**
Error: The `Metrics/LineLength` cop has been moved to `Layout/LineLength`.
(obsolete configuration found in .rubocop_todo.yml, please update it)

This error prevents contributors using newer Rubocop versions from running style checks or auto-corrections, creating a barrier to contribution.

🎯 What's Changed

✅ Compatibility Fixes

  • Fixed obsolete Rubocop configuration that was blocking newer versions
  • Updated deprecated cop names in .rubocop_todo.yml and .rubocop.yml
  • Ensured compatibility with modern Rubocop versions while maintaining functionality

✅ Code Quality Improvements

  • Resolved 48+ Rubocop style violations across multiple files
  • Applied auto-corrections for consistent code style
  • Zero functional changes - only style and formatting improvements

📁 Files Modified

Core Files

  • lib/httplog/http_log.rb - 44+ violations fixed
  • lib/httplog/configuration.rb - Layout and alias improvements
  • lib/httplog/version.rb - Removed unnecessary .freeze

Adapters

  • lib/httplog/adapters/ethon.rb - Lambda literal spacing
  • lib/httplog/adapters/httpclient.rb - Redundant begin blocks, conditional formatting

Configuration Files

  • .rubocop.yml - Updated deprecated cop names for newer Rubocop versions
    • Layout/IndentArrayLayout/FirstArrayElementIndentation
    • Updated TargetRubyVersion to match gemspec (>= 2.6)
  • .rubocop_todo.yml - Fixed obsolete configuration references
    • Metrics/LineLengthLayout/LineLength

🔍 Types of Violations Fixed

Layout & Formatting

  • ✅ Hash key alignment
  • ✅ Method definition parentheses
  • ✅ Conditional assignment patterns
  • ✅ Empty line management
  • ✅ Operator spacing

Style Improvements

  • ✅ Lambda literal spacing (-> (*)->(*)
  • ✅ String interpolation over concatenation
  • ✅ Modifier if usage for single-line statements
  • ✅ Redundant begin block removal
  • ✅ Alias syntax modernization

Compatibility Updates

  • Metrics/LineLengthLayout/LineLength
  • Layout/IndentArrayLayout/FirstArrayElementIndentation
  • ✅ Updated TargetRubyVersion to match gemspec requirement (>= 2.6)

🧪 Testing

  • ✅ All existing tests continue to pass
  • ✅ No functional changes made
  • ✅ Only style and formatting improvements applied
  • ✅ Compatible with CI/CD pipeline (no Rubocop checks in Travis)

📊 Before & After

Before

rubocop lib/
# Error: The `Metrics/LineLength` cop has been moved to `Layout/LineLength`.
# (obsolete configuration found in .rubocop_todo.yml, please update it)

After

rubocop lib/
# ✅ 10 files inspected, no offenses detected

🎉 Benefits

  1. 🔧 Removes Contribution Barriers - Contributors with newer Rubocop versions can now run style checks
  2. 📈 Improved Code Quality - Consistent style across the codebase
  3. 🔧 Better Maintainability - Easier to read and understand
  4. 👥 Enhanced Developer Experience - New contributors will have cleaner code to work with
  5. 🚀 Modernized Tooling - Compatible with newer Rubocop versions
  6. ⚡ Future-Proof - Prevents style violations in future contributions

🔗 Related Issues

This PR addresses a compatibility issue that was preventing contributors from using modern development tools, while simultaneously improving overall code quality.

🚨 Breaking Changes

None - This PR contains only style improvements and configuration updates with zero functional changes.

📝 Checklist

  • Code follows the project's style guidelines
  • Self-review of the code completed
  • No functional changes made
  • All existing tests pass
  • Rubocop violations resolved
  • Configuration files modernized for compatibility
  • Verified Rubocop works with newer versions

💬 Notes for Reviewers

This PR addresses both a practical compatibility issue and code quality improvements. The main driver was the obsolete Rubocop configuration that was blocking contributors using newer versions. While fixing that, I took the opportunity to resolve the existing style violations.

All changes are either:

  1. Configuration updates for Rubocop compatibility
  2. Auto-generated fixes from Rubocop
  3. Manual style improvements that don't affect functionality

Feel free to run the tests to confirm no functional changes were introduced! 🚀


Type: refactor | Priority: medium | Size: medium

@italomatos
Copy link
Contributor Author

italomatos commented Sep 18, 2025

Hey @trusche, could you please review this PR?
Thank you so much!

@trusche
Copy link
Owner

trusche commented Sep 18, 2025

Hi @italomatos , thank you for the work, but I have no intention of using rubocop on this project. In principle I'm all for it, but we're monkeypatching a lot of other gems here, and I want to mirror their syntax, idiosynchracies and all, as closely as possible.

I'm also not in favour of this style, personally, for lines of any length:

HttpLog.log_connection(site.host, site.port) if HttpLog.url_approved?("#{site.host}:#{site.port}")

It tends to hide the important bit, which is the condition, from a casual glance. So now we're getting into personal preferences. Tabs vs spaces.

@trusche trusche closed this Sep 18, 2025
@italomatos
Copy link
Contributor Author

Hi @italomatos , thank you for the work, but I have no intention of using rubocop on this project. In principle I'm all for it, but we're monkeypatching a lot of other gems here, and I want to mirror their syntax, idiosynchracies and all, as closely as possible.

I'm also not in favour of this style, personally, for lines of any length:

HttpLog.log_connection(site.host, site.port) if HttpLog.url_approved?("#{site.host}:#{site.port}")

It tends to hide the important bit, which is the condition, from a casual glance. So now we're getting into personal preferences. Tabs vs spaces.

it makes sense , thank you for quick review

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