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-5422 - Configuration DSL should not require an argument to its block (Rails parity) #5367

Merged
merged 6 commits into from
Jul 26, 2022

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Jul 10, 2022

This PR removes the need for |config| arg in Mongoid.configure. It makes it more terse and consistent with Rails.application.configure. The change is fully backwards compatible.

This syntax is now possible:

  Mongoid.configure do
    # can call methods by themselves:
    connect_to("mongoid_test")

    # `config` is defined internally, so this also works here:
    config.connect_to("mongoid_test")

    # must use `config.variable =` when assigning variables
    config.preload_models = true

Previous syntax (still supported):

  Mongoid.configure do |config|
    config.connect_to("mongoid_test")
    config.preload_models = true

To do this magic, like Rails, it uses instance_exec. For variable assignment, in the block, you need to use self.variable = "foo" (otherwise its a local variable.) To get around this, we introduce a method config that returns self, which is also what Rails does:

Rails.application.configure do
  config.cache_classes = true

Note this only affects assigning with =, so other methods work fine (such as connect_to in example above. (I now fully grok why Puma's config DSL uses only methods and not variable assignment.) Most IDEs will warn about local variable assignment in the block.

@johnnyshields johnnyshields changed the title MONGOID-5422 - Configuration DSL no longer requires an argument to its block MONGOID-5422 - Configuration DSL should not require an argument to its block (Rails parity) Jul 10, 2022
spec/mongoid_spec.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

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

If you can adjust the tests to correctly restore config options everything else looks good.

@johnnyshields
Copy link
Contributor Author

Done > using config_override now

@p-mongo p-mongo requested review from comandeo and Neilshweky July 21, 2022 14:23
Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

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

The release notes addition will conflict with #5396.

Copy link
Contributor

@Neilshweky Neilshweky left a comment

Choose a reason for hiding this comment

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

This is awesome! My one question is that it looks like config is now delegating to self. That means you could probably do something like:

Mongoid.configure do
  self.preload_models = true
end

Or even:

Mongoid.configure do
  preload_models = true
end

Two questions:

  1. Is that true?
  2. Does rails behave that way as well?

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Jul 21, 2022

@Neilshweky the behavior here is the same as Rails:

Mongoid.configure do
  self.preload_models = true # can do this
  config.preload_models = true # can also do this, because there is now a magic `config` method which returns self

  preload_models = true # cannot do this, because its a local variable assignment  
end

@johnnyshields
Copy link
Contributor Author

@p-mongo release note conflict resolved; ready to merge.

Configuration DSL No Longer Requires an Argument to its Block
-------------------------------------------------------------

In Mongoid 8.1, it is now possible to use ``Mongoid.configure`` without
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not add version numbers to release notes like this, it makes backporting changes more difficult and the release notes already specify the version they are for.

@p-mongo p-mongo merged commit 45ae318 into mongodb:master Jul 26, 2022
@johnnyshields
Copy link
Contributor Author

Thanks @p-mongo for getting this one to the finish line!

@johnnyshields johnnyshields deleted the MONGOID-5422-config-dsl branch July 26, 2022 15:07
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.

3 participants