Backport months of work to 4.1 #678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We would prefer to only constrain mysql2 to '~> 0.3', but a rails bug (rails/rails#21544) requires us to constrain to '~> 0.3.20' for now.
See previous commit.
Also, various minor formatting. [ci skip]
Because model_test.rb is too long, and it's misleading to define multiple classes in such a long file. It's easy to not notice that there's a second class. Also, clarify a few of the Has Many Through (HMT) tests - Improve descriptions - Decrease context nesting - Prefer local variables over instance variables - Name version variables more consistently - Use version numbers instead of `last`
This refactoring encapsulates the process of reification, separating it from the model (VersionConcern). The ABC complexity score of VersionConcern (measured by flog) is reduced from 750 to 362, a 52% reduction. Finally, this refactoring provides a truely private namespace for methods like `reify_has_many_through`. Such methods will no longer be mixed into PaperTrail::Version, or end-user's version models, if any.
Rails 5, in rails/rails@7644a99d, deprecated controller callbacks suffixed `_filter` in favour of those suffixed `_action`. However, callbacks of the latter variety don't exist in Rails 3.2, so check if they exist.
Reduce duplication between reify_has_many_directly and reify_has_many_through. This refactoring reduces the ABC complexity as follows: Before ------ 489.0: flog total 81.5: flog/method average 129.2: PaperTrail::Reifier::reify_has_many_through 114.9: PaperTrail::Reifier::reify_has_many_directly After ----- 454.2: flog total 64.9: flog/method average 95.5: PaperTrail::Reifier::reify_has_many_through 81.5: PaperTrail::Reifier::reify_has_many_directly
Reduce duplication between reify_has_many_directly and reify_has_many_through. This refactoring reduces the ABC complexity as follows: Before ------ 454.1: flog total 64.9: flog/method average 95.5: PaperTrail::Reifier::reify_has_many_through 81.5: PaperTrail::Reifier::reify_has_many_directly After ----- 444.3: flog total 55.5: flog/method average 85.5: PaperTrail::Reifier::reify_has_many_through 71.6: PaperTrail::Reifier::reify_has_many_directly
…per_trail_options[:on] when using callback-methods
It's still red, even though the build of master has been green for more than 24 hours.
Explanation from María de Antón at Travis CI: > The default badge image shows the latest build result > (https://api.travis-ci.org/airblade/paper_trail.svg), > which in this case is > https://travis-ci.org/airblade/paper_trail/builds/84872354 > (from: https://travis-ci.org/airblade/paper_trail/builds) > > If you check master branch's badge, it's indeed green: > https://api.travis-ci.org/airblade/paper_trail.svg?branch=master > as its latest build.
It was redundant because the table of contents has basically the same items. [ci skip]
[ci skip]
The basics section is more friendly, and a better introduction, so it should come first. I'm not at all sure we should even keep the API summary. It would be better to use something like RDoc for that. [ci skip]
The `private` keyword does, in fact, make those instance methods private. I think what I was trying to say is that the host class (the class that VersionConcern is mixed into) will be able to use these methods. They *will* be private after being mixed in, but they are not "api private", if that makes sense. [ci skip]
Replaces #662 [ci skip]
[ci skip]
Test would fail when FFaker::Color.name returned the same exact string twice in a row. This is not FFaker's fault. Incidentally, the FFaker color list currently has 19 entries. (https://github.com/ffaker/ffaker/blob/master/lib/ffaker/data/color/names_list) The test suite runs this postgres-specific test six times (3 rubies * 2 major versions of rails). So, I think the chance of the test suite failing was (1 / 19)^2 * 6, or 0.016, or about 2%.
It's not clear to me that random test data makes this particular test better.
Rails 4.2 deprecates `serialized_attributes` without replacement. However, it also introduces a type system which lets us treat all attributes the same. Rails 4.2 has `type_for_attribute` which knows how to serialize and deserialize itself from a database through `type_cast_for_database` and `type_cast_from_database`. (In Rails 5 they will be `serialize` and `deserialize` respectively.) Thus we no longer need the `PaperTrail.config.serialized_attributes` toggle, and this change makes it do nothing. It's still kept around for backwardscompatibility.
Tests were failing on MySQL due to timestamps with differing usec flunking `assert_equal`. Happened because Rails 4.2 added support for fractional seconds precision. Travis, however, doesn't support the required MySQL 5.6 for it. Bend the timestamps to strip out fractional seconds when tests are run with MySQL.
jaredbeck
force-pushed
the
backport_months_of_work_to_4.1
branch
from
81e3f55
to
8f11e67
Compare
Because the 4.1 branch still supports ruby 1.8.7 and flat_map was added in 1.9.2.
Because of ruby 1.8.7
|
Whew! I had to fix a few things that were incompatible with ruby 1.8.7, but I think this is finally ready for review. |
This is a follow-up to PR 667 "Use Active Record's type system from 4.2 onwards."
This fixes an intermittent test failure where query sometimes returned records in an order other than that in which they were created. This is not a bug. Without an order clause, SQL queries are expected to return records in any order.
|
Thanks to the eight humans and one robot (@orthographic-pedant) who have contributed so far to PT 4.1! 🎉 🎈 |
|
Hey @jaredbeck, I'm on holiday with my family this week so haven't had a ton of time to look at this stuff but will try to take a peek tonight. Looks good on the surface though, I'm guessing you would know better than I what makes sense to backport. |
Thanks, just let me know if you see anything that looks like a breaking change. Enjoy the holiday! 🎄 |
|
Mind if I merge this, Ben? |
| @@config ||= PaperTrail::Config.instance | ||
| yield @@config if block_given? | ||
| @@config | ||
| @config ||= PaperTrail::Config.instance |
There was a problem hiding this comment.
Was there a pro / con to storing this as an instance or class level variable? Is this being done to address thread safety concerns?
There was a problem hiding this comment.
It was changed from a class variable to a class instance variable. The change is purely stylistic, and has no effect on thread safety. See #641 for rationale.
jaredbeck
added a commit
that referenced
this pull request
Jan 8, 2016
Backport months of work to 4.1
|
Sorry @jaredbeck, never had a chance to go through all of the changes but everything looked like it was on point from what I did look through. Nice work. Can try to do some testing with you at some point next week if helpful. |
|
No worries, Ben. Hopefully a few people can try out the |
|
@jaredbeck sweet! I'll try pointing our Gemfile at |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@batter
This took a little while to prepare, and involved a few conflicts. In particular, I had to take care to avoid two things:
set_paper_trail_whodunnitcallback. Control the order ofset_paper_trail_whodunnitcallback #556Please let me know if I included anything I shouldn't have.