Adding additional configurable filters to tags

[ProGM]

Adding a configurable option to apply additional filters to the tags before inserting them.

It can be useful to apply regular expressions or other simple string modifications to tags.

Example:
If you configure it like this:

ActsAsTaggableOn.filters = { capitalize: [], gsub: ['o', 'a'] }

Your tags will look like this:

@user.tag_list = 'foo, bar'

@user.tag_list
# => ["Faa", "Bar"]

[seuros]

I personally find the syntax very hard to understand, i don't mind adding this feature if the more clear.
Also i don't like that the filter is global instead of query based.

@bf4, i need your opinion on this.

[ProGM]

@seuros mmh... you're right. What about something like:

ActsAsTaggableOn.filters = {
  {
    method: 'capitalize'
  },
  {
    method: 'gsub',
    parameters: ['o', 'a']
  }
}

And... what you mean for "query based filters"?

[seuros]

I want to do something like

tag_list = ActsAsTaggableOn::TagList.new('dogs', 'friendships', parser: MyParser)
tag_list = ActsAsTaggableOn::TagList.new('dogs', 'friendships', parser: MyOtherParser)

or

tag_list = ActsAsTaggableOn::TagList.new('dogs', 'friendships')
tag_list.parser #=> ActsAsTaggableOn::TagListParser
tag_list.to_s #=> ['dogs', 'friendships']
tag_list.parser = DogReplacerParser
tag_list.to_s #=> ['cats', 'friendships']

I started few month ago by extracting the parser in https://github.com/mbleigh/acts-as-taggable-on/blob/master/lib/acts_as_taggable_on/tag_list_parser.rb

[ProGM]

Uhm... I understand. And the idea is to make it configurable?

ActsAsTaggableOn.parser = DogReplacerParser # default: ActsAsTaggableOn::TagListParser

[seuros]

Correct.

ActsAsTaggableOn.default_parser = DogReplacerParser # default: ActsAsTaggableOn::TagListParser

[ProGM]

I can manage it if you want :)

What is the common way to do? Should I work on this pull request or create a new one?

[seuros]

Sure thing.

I was think we should have an GenericParser class which we use to inherit all other parsers

module ActsAsTaggableOn
##
# Returns a new TagList using the given tag string.
#
# Example:
# tag_list = ActsAsTaggableOn::GenericParser.new.parse("One , Two, Three")
# tag_list # ["One", "Two", "Three"]
class GenericParser
  def initialize(tag_list)
    @tag_list = tag_list
  end

  def parse
    @tag_list.split(',')
  end
end

DefaultParser < GenericParser

The default Default parser should keep the current behavior for now, we can drop to GenericParse in version 4.x

You can use this PR if you want.

[ProGM]

For DefaultParser you mean the actual ActsAsTaggableOn::TagListParser ?

So shound I turn ActsAsTaggableOn::TagListParser an instantiable class?

[seuros]

Yes. That module is too complex IMO. I think that users who have complex tags requirement, should implement the complex logic in their app instead of providing multi delimiter/multi quote parser by default.

If you feel that we can't do it without breaking existing applications. We could get start the version 4.0.0 next week with this feature. Wdyt ?

[ProGM]

I'm taking a look where the default parser is used right now... I think we can mantain compatibility.
The only thing is that probably ActsAsTaggableOn.delimiter will become useless...

Take a look to my last commit and tell me if it was what you intended to do.

P.S. I've not finished yet, of course :)

[seuros]

We can leave the TagListParser as module and delegate the parsing to the DefaultParser and change our code to use DefaultParser instead.
Code using TagListParser will have the deprecation warning.

[ProGM]

Yes, you're right.

One thing: should I leave the internal methods like delimiter, double_quote_pattern, single_quote_pattern from TagListParser?

[seuros]

The DefaultParser will have those.
TagListParser will have just the self.parse(string) method.

[seuros]

I agree with you, the delimiter will be deprecated then.
We can have a warning in delimiter=

[ProGM]

@seuros
Take a look to the last commit :) I hope it's ok!

Maybe tests needs to be reviewed a bit.

[seuros]

I think we should use ActsAsTaggableOn::DefaultParser

[ProGM]

Do you mean only here or everywhere?

So the tagged_with feature will always use the default parser?

I was quite uncertain about this while committing...

[seuros]

Sorry, you are right 👍 . I got confused after reading the spec file

[seuros]

@ProGM , seem we are good to merge 💚 . Could you squash your commits and update the Readme with a small example ?

[bf4]

well, this is a little weird.. commenting it out and then re-implementing it?

[ProGM]

What do you mean? It's the same implementation taken from TagListParser, moved here. :)

[seuros]

@bf4, It explain the regex used in the method below.

[bf4]

Great PR and tests, but I don't think the new classes improve the design or understandability of the code.. at the end of the day, @seuros do you want to maintain it? I think the first commit, which was must simpler, is a better first implementation.

[ProGM]

@seuros
ok, I'll add an example :)
What do you mean for "squash your commits"?
_EDIT_ Ok, I see what it's "squashing"... I never did it, I have read a guide ... :D

@bf4
uhm... ok, please take an unique decision all together and tell me what to do, so I avoid to waste my time ;)

[ProGM]

@seuros
I found a little bug in my implementation:

@user = User.new(:name => "Bobby")
ActsAsTaggable.default_parser = MyParser
@user.tag_list = "east, south"
ActsAsTaggable.default_parser = MyParser2
@user.tag_list.add("north, west")
# => This will use MyParser instead of MyParser2!

I try to fix.

[seuros]

@bf4
yes, sure. I will have not asked @ProGM for modifications if i wasn't going to maintain it.
With the classes, parsing become very flexible. You can for example translate/correct/validate/add/remove the tags on the fly. For example we could have a ColorParser that will change all English color words into their hex variant. You can have another parser that remove swear words.
There was no way to do it previously.

@ProGM
I don't think it a bug

@user = User.new(:name => "Bobby")
ActsAsTaggable.default_parser = MyParser
@user.tag_list = "east, south"
other_tags = MyParser2.new("north; west")
@user.tag_list.add(other_tags) #=> Add should not parse if the object is a taglist.

i think that changing the default parser will cause another thread to use it and we will have unexpected results. WDYT ?

[ProGM]

@seuros Oh, you are right!
Just one more thing: The default parser returns a TagList, the GenericParser returns an array. Isn't it quite confusing for an user that is trying to implement his own parser?
Should we add an example implementation of a custom parser?
Please check examples I added in the Readme. I don't trust my english :P

[seuros]

Can we change it to return a Taglist by default ?

Should we add an example implementation of a custom parser?

i think you did it in line 252-262

Please check examples I added in the Readme.

LGTM.

[ProGM]

@seuros
Yeah, I can change it
And a small thing.
Actually if you do:

@user = User.new(:name => "Bobby")
@user.tag_list = "east, south"
@user.tag_list.add("north; west", parser: MyParser) #=> This will do nothing, because the `parse: true` is not specified

parse: true should be implicit if you pass a parser as parameter... WDYT?

[seuros]

👍 Sound good

[ProGM]

@seuros
ok, done :)

[seuros]

💚 💚 💚 💚 Nice. Can you squash the commits ? Do you need help with that ?

[ProGM]

@seuros Yes, if you can help me with that... I don't want to make a mess! :)

[seuros]

Ok

git rebase d2144bb6e0aa75467eb7edf1655183cd752b9ff7^1 -i     

The sha1 is the one of the first commit , ^1 tells git to use that commit also.

you will be presented with a list of commit , change all the pick to squash except the first one.
That will squash them all into the 1 commit. Save and exit. (:wq (vim) or ctrl+x (nano))

You will be presented to edit the new commit message (by default it will contact all the commits message). Delete everything and write a descriptive comment. Save and exit.

git log 
# check if there is now just 1 commit
git push origin -f
# That will force push the change into your branch.

[ProGM]

@seuros I think it worked. Thank you!

[seuros]

Yep. I will merge once travis go green.

[ProGM]

👍

[bf4]

Congrats!!