Skip to content
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

MONGOID-5809: remove ruby -w warnings #5921

Merged

Conversation

DarshanaVenkatesh
Copy link
Contributor

@DarshanaVenkatesh DarshanaVenkatesh commented Dec 13, 2024

MONGOID-5809
When run with ruby -w, Mongoid is fairly noisy and provides complaints about method redefinitions and overrides. Remove warnings by preventing redefinition.

Original Warnings:
Screenshot 2024-12-13 at 2 10 59 PM

@DarshanaVenkatesh DarshanaVenkatesh force-pushed the 5809-remove-ruby-warnings branch from 45ed9a3 to 7857378 Compare December 13, 2024 22:34
Comment on lines 16 to 17
remove_method :include_root_in_json if method_defined?(:include_root_in_json)
remove_method :include_root_in_json= if method_defined?(:include_root_in_json=)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I had to dig a bit to understand that these methods are defined in ActiveModel, and we're just re-defining them here to account for our default value Mongoid.include_root_in_json. Maybe add a comment to that effect?

Also, there's an extra space before these lines -- we've standardized on indentations in 2-space increments.

class << self
remove_method :discriminator_key if method_defined?(:discriminator_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here -- please add a comment explaining that the preceding class_attribute creates both a getter and a setter, but we need to reimplement the getter (by delegating).

Also, the if method_defined?(...) is unnecessary here, since we know that the preceding class_attribute ensures the method is defined. 👍

Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

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

💯

@DarshanaVenkatesh DarshanaVenkatesh merged commit 3cefc7d into mongodb:master Dec 17, 2024
67 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.

2 participants