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

Multiple quoted tags #394

Closed
wants to merge 2 commits into from
Closed

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Aug 26, 2013

This is a bug @thatbettina and I came across when tags are entered using quotes, and there are multiple of them after one another, then things don't work as expected.

This PR adds a test case and a fix.

The problem is that the regular expression that matches quoted tags matches a delimiter at the start and at the end. Because of this, when matching multiple consecutive quoted tags, the delimiter in between is already matched as end-delimiter for the first tag, and so can no longer serve as start-delimiter for the second tag.

By using zero-width lookahead in the regexp, the second delimiter is not consumed.

The PR also limits the Rails version again to >= 3.0, < 4.0 because we got unrelated errors when testing on Rails 4, so we couldn't verify the feature works otherwise. This also seems to be why the build on Travis is currently broken.

@bf4
Copy link
Collaborator

bf4 commented Dec 10, 2013

Would you mind rebasing against master and force-pushing? Thanks

thatbettina and others added 2 commits December 10, 2013 17:29
The regular expression that matches quoted tags matches a delimiter at the start
and at the end. Because of this, when matching multiple consecutive quoted tags,
the delimiter in between is already matched as end-delimiter for the first tag,
and so can no longer serve as start-delimiter for the second tag.

By using zero-width lookahead in the regexp, the second delimiter is not
consumed.
@plexus
Copy link
Contributor Author

plexus commented Dec 10, 2013

yay :)

rebase done

cc @thatbettina

@@ -22,7 +22,7 @@ Gem::Specification.new do |gem|
gem.post_install_message = File.read('UPGRADING')
end

gem.add_runtime_dependency 'rails', ['>= 3', '< 5']
gem.add_runtime_dependency 'rails', ['>= 3', '< 4']

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see PR description

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests on master are now passing for Rails 4. Please amend commit and remove this change.

Also, for the future, such a huge change that should have been in a separate commit and highlighted in the PR description.

@seuros
Copy link
Collaborator

seuros commented Mar 30, 2014

@plexus : Do you rebasing this PR ? I can set it for version 3.2.0

@bf4 bf4 closed this in 95998f6 Apr 2, 2014
@bf4
Copy link
Collaborator

bf4 commented Apr 2, 2014

@seuros I finished this and annotated it myself. Could be better, but is done, at least.

@seuros seuros added this to the 3.2.0 milestone Apr 2, 2014
tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
The regular expression that matches quoted tags matches a delimiter at the start
and at the end. Because of this, when matching multiple consecutive quoted tags,
the delimiter in between is already matched as end-delimiter for the first tag,
and so can no longer serve as start-delimiter for the second tag.

By using zero-width lookahead in the regexp, the second delimiter is not
consumed.

Conflicts:
	lib/acts_as_taggable_on/tag_list.rb

bf4 - Modified and annotated expanded regex
Closes mbleigh#394
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.

5 participants