Hook to support STI subclasses of Tag in save_tags #413

ches · 2013-09-26T19:43:01Z

One use case I have is that a custom Tag class is "followable" (see way back to #218). Because Core#save_tags is a monstrous method, I was hacking around it on an override using with_scope around super -- this eliminates the need for such a dirty trick.

This is possibly incomplete support for Tag being robustly subclass-able, but I haven't run into any other issues yet with the features of acts-as-taggable-on that we're using.

bf4 · 2013-12-10T18:27:14Z

Can you rebase against master and force push?

ches · 2013-12-10T19:12:34Z

Rebased.

bf4 · 2013-12-10T19:41:14Z

spec/schema.rb

@@ -1,6 +1,7 @@
 ActiveRecord::Schema.define :version => 0 do
  create_table :tags, :force => true do |t|
    t.string :name
+    t.string :type


This column is only for the purpose of the STI tests, right? You're otherwise adding it yourself?

Right, didn't want to make any changes to default generators and such since it isn't required for the basic, out-of-the-box plugin behavior, but I thought supporting extensibility and having at least light test coverage for it was worthwhile (I've done a lot more to support extensibility elsewhere, because I needed it, but it could use more tests before upstream submission).

For completeness, I think, in the test, we should also set self.inheritance_column :type so that we can explicitly set the STI column we'll be switching on. We should probably also check for the STI column in the code

That sounds reasonable about inheritance_column. What were you thinking for "check for the STI column in the code"? ActiveRecord probably lends some help with warnings/errors, but I can double-check, if any specific scenarios come to mind.

bf4 · 2013-12-27T15:51:45Z

I'm really unsure if this is good functionality for the library as commented in the related issue.

ches · 2013-12-28T08:29:53Z

I'm really unsure if this is good functionality for the library as commented in the related issue.

Well, that's certainly the maintainer's judgment call to make, whether a "plugin" has different extensibility goals than a "library" basically. If that is ultimately the decision, then I'll be maintaining a fork for the foreseeable future, because we needed the functionality in a real-world use case (more than one of them actually, but my example in #218 of followable tags is the most widely pertinent to offer as an example).

Personally, I think it'd be kind of sad if a tagging library made a judgment that its users would never want to subclass its Tag class for richer behavior, but if you need to limit what you support because of maintenance effort required, I'm empathetic to that in general.

bf4 · 2013-12-28T15:55:23Z

See if you can get it so there are no unused arguments or variables... I know the code is hard to work with

bf4 · 2013-12-31T16:33:25Z

lib/acts_as_taggable_on/tag.rb

@@ -72,13 +72,13 @@ def self.find_or_create_all_with_like_by_name(*list)

      return [] if list.empty?

-      existing_tags = Tag.named_any(list)


Ah, this is definitely a good call. I missed that this was a subclassing issue unrelated to the STI issue.

bf4 · 2014-01-10T22:27:54Z

Alright. Make sure all variables are used and we are gtg

bf4 · 2014-01-13T15:27:06Z

Merged in more commits to master, including your OCD branch, so this needs and update, ensure all variables are used, and its good to go

seuros · 2014-04-18T12:57:53Z

@bf4, are we still going to merge this PR ? LGTM , it just need a rebase.

ches · 2014-04-18T18:20:51Z

I'll try to take a look at rebasing this again shortly. As noted in earlier discussion above though, regarding the insistence on not having an unused variable in the form of a parameter: the context parameter was basically my entire reason for wanting a hook, so knowledge of it can be used in extension situations -- see the test case added here with Market and company.market_list= for an example use case.

One use case I have is that a custom Tag class is "followable". Because `Core#save_tags` is a monstrous method, I was dirtily hacking around it on an override using `with_scope` around `super` -- this eliminates the need for such a trick.

ches · 2014-04-18T20:13:19Z

Okay, rebased again.

One thing to note though from the interim: #371 extracted the load_tags method, with a quite similar motivation to my own, except that I also want awareness of context when extending. While rebasing here, I kept load_tags for compatibility, because it's now public API that's been released. It's redundant/a needless level of indirection to have both load_tags and my hook method, but I'm not sure how you'd want to approach backwards compatibility (as a reminder, before rebasing, my hook method's body was exactly the same as load_tags's body). Overriding load_tags is insufficient for my use case because, again, there's no way to get a reference to the context being used when overriding it.

Though mine is long, I think load_tags is a poor name because it does more than that: it might have the side effect of creating, not just loading. FWIW #371 also did not address the tag ownership module, which my changes did.

Hook to support STI subclasses of Tag in save_tags

bf4 reviewed Dec 10, 2013
View reviewed changes

bf4 reviewed Dec 31, 2013
View reviewed changes

seuros added waiting on response labels Apr 18, 2014

ches added 2 commits April 19, 2014 02:50

Apply Tag STI hook for owned taggings

d5889dd

ches referenced this pull request Apr 18, 2014

Removed ruby deprecated syntax

4c5314b

seuros added a commit that referenced this pull request Apr 18, 2014

Merge pull request #413 from ches/hook-for-tag-sti-subclass

e6a5da4

Hook to support STI subclasses of Tag in save_tags

seuros merged commit e6a5da4 into mbleigh:master Apr 18, 2014

seuros removed waiting on response labels Apr 18, 2014

seuros added this to the 3.2.0 milestone Apr 18, 2014

ches deleted the hook-for-tag-sti-subclass branch November 7, 2015 01:46

tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021

Merge pull request mbleigh#413 from ches/hook-for-tag-sti-subclass

19eaf68

Hook to support STI subclasses of Tag in save_tags

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook to support STI subclasses of Tag in save_tags #413

Hook to support STI subclasses of Tag in save_tags #413

ches commented Sep 26, 2013

bf4 commented Dec 10, 2013

ches commented Dec 10, 2013

bf4 Dec 10, 2013

ches Dec 10, 2013

bf4 Dec 31, 2013

ches Jan 9, 2014

bf4 commented Dec 27, 2013

ches commented Dec 28, 2013

bf4 commented Dec 28, 2013

bf4 Dec 31, 2013

bf4 commented Jan 10, 2014

bf4 commented Jan 13, 2014

seuros commented Apr 18, 2014

ches commented Apr 18, 2014

ches commented Apr 18, 2014

		@@ -72,13 +72,13 @@ def self.find_or_create_all_with_like_by_name(*list)

		return [] if list.empty?

		existing_tags = Tag.named_any(list)

Hook to support STI subclasses of Tag in save_tags #413

Hook to support STI subclasses of Tag in save_tags #413

Conversation

ches commented Sep 26, 2013

bf4 commented Dec 10, 2013

ches commented Dec 10, 2013

bf4 Dec 10, 2013

Choose a reason for hiding this comment

ches Dec 10, 2013

Choose a reason for hiding this comment

bf4 Dec 31, 2013

Choose a reason for hiding this comment

ches Jan 9, 2014

Choose a reason for hiding this comment

bf4 commented Dec 27, 2013

ches commented Dec 28, 2013

bf4 commented Dec 28, 2013

bf4 Dec 31, 2013

Choose a reason for hiding this comment

bf4 commented Jan 10, 2014

bf4 commented Jan 13, 2014

seuros commented Apr 18, 2014

ches commented Apr 18, 2014

ches commented Apr 18, 2014