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

[READY FOR REVIEW] MONGOID-5336: User-defined symbol field types #5269

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johnnyshields
Copy link
Contributor

No description provided.

docs/reference/fields.txt Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
spec/mongoid/errors/invalid_field_type_spec.rb Outdated Show resolved Hide resolved
lib/config/locales/en.yml Outdated Show resolved Hide resolved
docs/reference/fields.txt Outdated Show resolved Hide resolved
@johnnyshields
Copy link
Contributor Author

@p-mongo I believe I have resolved all your requested changes. Please review again, thanks.

lib/config/locales/en.yml Outdated Show resolved Hide resolved
lib/config/locales/en.yml Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
spec/mongoid/errors/invalid_field_type_spec.rb Outdated Show resolved Hide resolved
spec/mongoid/errors/invalid_field_type_spec.rb Outdated Show resolved Hide resolved
spec/mongoid/errors/invalid_field_type_spec.rb Outdated Show resolved Hide resolved
spec/mongoid/fields/field_types_spec.rb Show resolved Hide resolved
spec/mongoid/fields/field_types_spec.rb Show resolved Hide resolved
@johnnyshields
Copy link
Contributor Author

Have done all requested second round changes. Appreciate if you can resolve the comments once you check them.

lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
lib/mongoid/fields/field_types.rb Outdated Show resolved Hide resolved
@johnnyshields johnnyshields changed the title MONGOID-5336: User-defined symbol field aliases MONGOID-5336: User-defined symbol field types Jun 24, 2022
@p-mongo
Copy link
Contributor

p-mongo commented Jul 9, 2022

I like the block configuration syntax but it should be on top-level Mongoid module, e.g. Mongoid.configure do ... not nested under fields.

The actual changes pertaining to alias definition I think are mostly OK.

I'm not seeing the purpose of maintaining two mappings - the "default" Mongoid one and the active one. Can you explain this part please?

@johnnyshields
Copy link
Contributor Author

I like the block configuration syntax but it should be on top-level Mongoid module.

Sure, will be glad to do this change.

I'm not seeing the purpose of maintaining two mappings - the "default" Mongoid one and the active one. Can you explain this part please?

Mongoid::Fields::FieldTypes::DEFAULT_MAPPING is an immutable constant. The active mapping Mongoid::Fields::FieldTypes.mapping (singleton) is initialized to a dup of this constant, and is mutable so users can add their own types. It would of course be possible to inline the constant into the .mapping method, but I thought it was cleaner to keep it separate (plus we need it for the deprecated Mongoid::Fields::TYPE_MAPPINGS constant.)

Does this answer your question?

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Jul 10, 2022

@p-mongo config change done, ready for a final review.

Note the config will look like this:

  Mongoid.configure do |config|
    config.define_field_type :point, Point
  end

To keep consistency, I'm also adding config.define_field_option in this PR.

I've raised https://jira.mongodb.org/browse/MONGOID-5422 (PR #5367) to support doing configure without the |config| arg. It can be done in a backwards compatible manner.

@johnnyshields
Copy link
Contributor Author

Tests are green 👍

docs/reference/fields.txt Outdated Show resolved Hide resolved
docs/reference/fields.txt Outdated Show resolved Hide resolved
@johnnyshields johnnyshields marked this pull request as draft July 13, 2022 05:35
@johnnyshields johnnyshields marked this pull request as ready for review October 4, 2022 19:43
@johnnyshields
Copy link
Contributor Author

Please kindly remove the "Oleg Responded" / "Oleg Todo" flags on this and all other PRs.

@johnnyshields johnnyshields changed the title MONGOID-5336: User-defined symbol field types [READY FOR REVIEW] MONGOID-5336: User-defined symbol field types Feb 10, 2023
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