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

Revert repeat includes #411

Closed
wants to merge 3 commits into from
Closed

Conversation

ches
Copy link
Contributor

@ches ches commented Sep 26, 2013

Rationale is given, with references, in the extended commit messages on this branch.

ches referenced this pull request Sep 26, 2013
@ches
Copy link
Contributor Author

ches commented Sep 26, 2013

I'll be happy to look further into the matter mentioned in 57fc9a9 of updating the macro hooks everywhere else that the change may be needed, when I have some more time to see if there is missing coverage, etc.

The Rails 4 build errors on Travis for this changeset should be resolved by #410, looks like Rails 4 has removed internal use of mattr_accessor, so we're no longer getting it transiently and needed an explicit require. I didn't want to combine the branches. #410 does appear to expose Rails 4 failures with SQLite and Postgres, but I'm pretty sure those aren't my bugs 😉

This reverts commit e44b69d.

This undid the work of 16c4567 such that we were now
re-including most the library on subsequent calls of the macro method
like:

    acts_as_taggable_on :pros
    acts_as_taggable_on :cons

The change was made in order to fix dirty tracking on mbleigh#321, but the
proper fix is that the dirty tracking module is missing a hook
`acts_as_taggable_on` macro that calls super.
Following b9b2747, a spec broke for related when ordered tags are in
use, since the hook for repeated calls of the macro was not maintaining
the preserve order value.

This hook pattern likely needs to be updated throughout the library now
that the `acts_as_ordered_taggable_on` exists as an alternative macro,
but I'm just fixing what broke a spec within the scope of my current
branch.
@ches
Copy link
Contributor Author

ches commented Dec 10, 2013

Rebased since unrelated build problems have been fixed since this originated.

@bf4
Copy link
Collaborator

bf4 commented Dec 19, 2013

I'll get to it...

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants