Skip to content

Conversation

@IngusSkaistkalns
Copy link

Overwrite options validation method instead of altering constant.

Of course I see drawbacks in this solution - now option versioned is not passed inside everywhere where COMMON is called.

Also I see possible issue with other extensions that also want to have their options to be valid - in this approach we would have conflicts.

In ideal scenario I envision that there is valid_relation_options method, that would be extendable by any other gems with something like super + [:my_options] or in mongoid change COMMON options constant to a class method....

Also I was not able to run tests successfully since there is dependency on mongoid-paranoia, but it has no release for mongoid-6

VERSIONED_OPTIONS = [:versioned].freeze

def validate!(options)
valid_options = options[:relation]::VALID_OPTIONS + COMMON + VERSIONED_OPTIONS
Copy link
Author

Choose a reason for hiding this comment

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

I would like something like this here -> options[:relation]::VALID_OPTIONS + common_options(),
where

def self.common_options
    super + [:versioned]
end
# or maybe have DSL for extending common options dedicated for extnsions
Mongoid::Relations::Options.add_common_option(:versioned)

, but all that requires changes in mongoid itself....

Copy link

@ebeigarts ebeigarts Jul 19, 2017

Choose a reason for hiding this comment

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

I think it would be less intrusive to use

alias_method :validate_without_versioned!, :validate!

def validate!(options)
  validate_without_versioned!(options - [:versioned])
end

Copy link
Author

Choose a reason for hiding this comment

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

@ebeigarts yes, looks definetly cleaner, only in case of invalid option exception message contains all valid options, but in this case versioned wont be included in exception message.

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