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

allow modifier field to be optional #205

Merged
merged 2 commits into from
Dec 7, 2017
Merged

allow modifier field to be optional #205

merged 2 commits into from
Dec 7, 2017

Conversation

yads
Copy link
Contributor

@yads yads commented Nov 30, 2017

No description provided.

@yads
Copy link
Contributor Author

yads commented Nov 30, 2017

fix for #186

@dblock
Copy link
Collaborator

dblock commented Dec 1, 2017

Looks good, needs specs, changelog, docs.

@yads yads force-pushed the master branch 2 times, most recently from 4bebf51 to 50f4bfb Compare December 1, 2017 14:59
@coveralls
Copy link

coveralls commented Dec 1, 2017

Coverage Status

Changes Unknown when pulling 50f4bfb on yads:master into ** on mongoid:master**.

@yads
Copy link
Contributor Author

yads commented Dec 1, 2017

@dblock it looks like the optional association option is only available in Mongoid 6. Which this feature needs to work properly on Mongoid 6. Are you aware of how to do feature/version detection on which version of Mongoid this would be running against? I supposed I could also do something like:

begin
        belongs_to :modifier, class_name: Mongoid::History.modifier_class_name, optional: true
rescue
        belongs_to :modifier, class_name: Mongoid::History.modifier_class_name
end

Do you have any thoughts on how to handle this situation so that this feature works on mongoid versions < 6 and >= 6?

@dblock
Copy link
Collaborator

dblock commented Dec 2, 2017

You can use checks in Mongoid::Compatibility to see what version is being used in both the implementation and the specs, so no rescue please.

@coveralls
Copy link

coveralls commented Dec 4, 2017

Coverage Status

Changes Unknown when pulling 13063fa on yads:master into ** on mongoid:master**.

@yads
Copy link
Contributor Author

yads commented Dec 4, 2017

@dblock added some specs and using mongoid-compatibility for feature detection. The only thing I had to up the rubocop module size as tracker.rb was right on the cusp of exceeding 176.

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Getting close! Hang in there. There're no tests for the new modifier_field_optional option, please add.

@@ -18,6 +18,7 @@ when /3/
else
gem 'mongoid', version
end
gem 'mongoid-compatibility'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to become a .gemspec dependency, otherwise it will be failing at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's already in the .gemspec file. Is this not it? https://github.com/mongoid/mongoid-history/blob/master/mongoid-history.gemspec#L23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or are you saying to add a version dependency so that it works with version 0.5.1 or above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then you don't need to add it here, don't you?

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage decreased (-8.8%) to 90.996% when pulling 1b3c42d on yads:master into d6102a1 on mongoid:master.

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage increased (+0.002%) to 99.762% when pulling 103dcb3 on yads:master into d6102a1 on mongoid:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.762% when pulling 103dcb3 on yads:master into d6102a1 on mongoid:master.

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage increased (+0.002%) to 99.763% when pulling 01fd3c8 on yads:master into d6102a1 on mongoid:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.763% when pulling 01fd3c8 on yads:master into d6102a1 on mongoid:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.763% when pulling 01fd3c8 on yads:master into d6102a1 on mongoid:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.763% when pulling 01fd3c8 on yads:master into d6102a1 on mongoid:master.

@yads
Copy link
Contributor Author

yads commented Dec 5, 2017

Yay, finally passing!

@@ -55,7 +55,7 @@ Metrics/MethodLength:
# Offense count: 2
# Configuration parameters: CountComments.
Metrics/ModuleLength:
Max: 176
Max: 200
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fyi usually we just run rubocop -a ; rubocop --auto-gen-config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, I was wondering why 176 was the old value :)

@dblock
Copy link
Collaborator

dblock commented Dec 7, 2017

Merging this, thanks!

@dblock dblock merged commit 487d4fe into mongoid:master Dec 7, 2017
@dblock
Copy link
Collaborator

dblock commented Jun 18, 2018

Coming from #221.

Is the modifier still not actually enforced in Mongoid 6? A spec like https://github.com/mongoid/mongoid-history/blob/master/spec/unit/attributes/destroy_spec.rb never uses a modifier but succeeds in 6 and now fails in 7 saying a modifier is required.

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