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

fix README: add parse:true documentation #386

Merged
merged 2 commits into from
Jan 2, 2014
Merged

Conversation

billychan
Copy link
Contributor

I found error when I followed README to add tags as

@user.tag_list.add("awesomer, slicker") 

The result is one tag named "awesome, slicker" which is incorrect. I browsed source and found the right usage should be

@user.tag_list.add("awesomer, slicker", parse: true)
# or
@user.tag_list.add("awesomer", "slicker")

The result is correct, two tags "awesome" and "slicker"

This pull request fixed the missing part in README.

@bf4
Copy link
Collaborator

bf4 commented Dec 10, 2013

Can you rebase against master and force push?

@billychan
Copy link
Contributor Author

@bf4, sure. Done.

@ghost ghost assigned bf4 Dec 11, 2013
@bf4
Copy link
Collaborator

bf4 commented Dec 27, 2013

Maybe add a note that this depends on the ActsAsTaggableOn.delimiter being set to ,? Also, please rebase and forced push to only one commit.

@billychan
Copy link
Contributor Author

@bf4, sure thing! Also I want to change all of the comment style in that block from inline comment to separated lines as this line on working and the last one are too long for readability. Is that okay?

@bf4
Copy link
Collaborator

bf4 commented Dec 27, 2013

Sure

@billychan
Copy link
Contributor Author

@bf4, I tried to add the dependency words but found there are several places with similar case. Doing such would looks redundant so I ended adding a general note above "#Usage". rebased against the latest master and force pushed.

@user.tag_list.remove("awesome", "slick")

# "remove" works with string too, needing "parse: true" option.
@user.tag_list.remove("awesome, slick", parse: true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

alternatively, rather than passing in an array of tag arguments, you can pass in a string with a delimited list of tags to be parsed, with 'parse: true'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe there should just be a general section on the options hash most of the tagging methods take, separate from the general examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea. I think the alternative section is cleaner, just like the line "To preserve the order in which tags are created use acts_as_ordered_taggable" and its later section. I'll do that.

@billychan
Copy link
Contributor Author

Hi @bf4, sorry for my late. I added quite a lot of changes on "Usage". Please take a look at first, I'll rebase it if everythings okay. Happy new year!

@bf4
Copy link
Collaborator

bf4 commented Dec 31, 2013

Pretty good! I might want to re-read for grammar, but content looks good.

bf4 added a commit that referenced this pull request Jan 2, 2014
[Doc] README: add parse:true documentation
@bf4 bf4 merged commit ad5c826 into mbleigh:master Jan 2, 2014
@billychan billychan deleted the fix-readme branch January 2, 2014 18:15
@billychan
Copy link
Contributor Author

Thanks!

tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
[Doc] README: add parse:true documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants