Skip to content

Fix warning message and add details in message. #2593

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

Merged
merged 1 commit into from
Aug 19, 2025

Conversation

ericproulx
Copy link
Contributor

Fix Registry Warning Message and Add Comprehensive Test Coverage

Summary

This PR improves the warning message in the Grape::Util::Registry module and adds comprehensive test coverage to ensure the registry functionality works correctly across various scenarios.

Changes Made

1. Enhanced Warning Message

File: lib/grape/util/registry.rb

  • Before: warn "#{short_name} is already registered with class #{klass}"
  • After: warn "#{short_name} is already registered with class #{registry[short_name]}. It will be overridden globally with the following: #{klass.name}"

The warning message now provides more detailed information by:

  • Showing the existing class that was previously registered
  • Clarifying that the registration will be overridden globally
  • Including the name of the new class being registered

2. Comprehensive Test Suite

File: spec/grape/util/registry_spec.rb (new file)

Added 186 lines of comprehensive test coverage including:

Core Functionality Tests

  • Registration of classes with different naming patterns (simple, camel case, namespaced)
  • Proper handling of invalid class names (anonymous, nil, empty)
  • Indifferent access for registry keys (string vs symbol)

Duplicate Registration Tests

  • Warning behavior when registering duplicate short names
  • Verification that warnings include correct class information
  • Confirmation that existing registrations are properly overwritten
  • Multiple duplicate registration scenarios

Edge Cases

  • Classes with special characters and multiple modules
  • Classes with numbers in names
  • Classes with acronyms (API, HTTP, etc.)

Why This Change Matters

  1. Better Debugging: The enhanced warning message provides developers with more context when duplicate registrations occur, making it easier to identify and resolve conflicts.

  2. Improved User Experience: Users of the Grape framework will now receive clearer feedback about what's happening during registration conflicts.

  3. Comprehensive Testing: The new test suite ensures the registry functionality is robust and handles edge cases properly, preventing regressions in future changes.

Testing

The changes include comprehensive test coverage that verifies:

  • ✅ All existing functionality continues to work
  • ✅ Warning messages are properly formatted and informative
  • ✅ Edge cases are handled gracefully
  • ✅ Registry behavior is consistent across different class naming patterns

Breaking Changes

None. This is a backward-compatible improvement that only enhances the warning message and adds test coverage.

Related Issues

Fix #2591

@grape-bot
Copy link

grape-bot commented Aug 16, 2025

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

@ericproulx ericproulx force-pushed the fix_registry_warning_message branch from 9e67af0 to 8bc2894 Compare August 16, 2025 11:31
@ericproulx ericproulx requested a review from dblock August 16, 2025 11:31
@ericproulx ericproulx marked this pull request as ready for review August 16, 2025 11:31
@ericproulx ericproulx force-pushed the fix_registry_warning_message branch from 8bc2894 to 425c835 Compare August 17, 2025 09:36
@dblock dblock merged commit 7cfc817 into master Aug 19, 2025
93 checks passed
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.

Dynamic registration of error formatters causes global overrides
3 participants