Fix issues with dirty plugin by dynamically defining dirty methods #348
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
shioyama
changed the title
|
@pwim This should roughly do it, minus a few more clean up changes. This is still on As described earlier, naming methods on the tracker carefully pays off because we can dynamically define all the attribute methods using introspection and just delegate to the tracker now. Seems to work ok in all versions, and should quietly support future changes as well. |
shioyama
force-pushed
the
fix_saved_changes
branch
2 times, most recently
from
321f6ce to
db48ec7
Compare
Previous to the dirty module refactor, attribute handlers like +attribute_changed?+ were supported, but with that change they became unsupported. We need to support the public ones at least. First, let's lay down good test coverage before making more changes. Also, ensure that methods which include both translated and untranslated attributes are tested against untranslated attribute changes as well, to ensure we haven't broken any basic functionality.
…odules The dirty tracking methods defined by ActiveModel and ActiveRecord change from version to version, which makes it tricky to correctly support all of them for translated attributes. Rather than adding many version conditionals to Mobility code, we can instead dynamically determine the dirty method patterns by including the AR/AM dirty module into a dummy class, and extracting the attribute methods from that class. We can then use those patterns to define Mobility dirty methods. However, to support both methods like +title_changed?+ and *also* +attribute_changed?(:title)+, we need to streamline things a bit. We can do this by changing the naming of methods on the Mobility mutation tracker such that its methods correspond exactly to the attribute handler methods. Doing this allows us to dynamically define both handler method overrides and dirty methods on translated attributes with some simple metaprogramming, in a way that will be mostly robust to changes in Rails (just need to add new methods to the dirty tracker to support new dirty methods on AM/AR). The result is that the Mobility ActiveRecord plugin becomes much simpler, whereas the ActiveModel one becomes slightly more complex, although most of that added complexity is in the mutation tracker, which is easy to understand. Tests are now very important since we are defining many more overrides than before (attribute handlers were not previously being overridden, but now we have no choice but to do so.)
shioyama
force-pushed
the
fix_saved_changes
branch
from
cea14c1 to
011575d
Compare
|
Ok, squashed commits and added more test coverage. Once specs pass, I'll merge and update (or maybe replace) #347, then we can finally release this as 0.8.9. |
|
Merged! Now just need to rebase 0-8-stable... |
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.
Resolving issues raised in #347, for master branch. For a summary see here, and 011575d.