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

Fixed parsing several filter arguments with quotes #98

Merged
merged 1 commit into from
Apr 18, 2017
Merged

Fixed parsing several filter arguments with quotes #98

merged 1 commit into from
Apr 18, 2017

Conversation

ilyapuchka
Copy link
Collaborator

@ilyapuchka ilyapuchka commented Jan 24, 2017

Current version (latest master) incorrectly parses subsequent filter parameters that use quotes, i.e. name|replace:\"foo\",\"bar\" will give three arguments instead of 2.

Internally it creates filter expression with three variables, "foo", ":" and "bar". It inserts unneeded colon because when splitting filter token by colon it splits it into replace, "foo" and ,"bar", then it drops the first and joins all others with colon. That gives arguments string "foo":,"bar". Then it also incorrectly splits it into "foo", : and "bar".

With this fix token is correctly split into replace, "foo","bar" and then arguments are split into "foo" and "bar"

@@ -7,18 +7,22 @@ extension String {
var word = ""
var components: [String] = []
var separate: Character = separator
var singleQuoteCount = 0
var doubleQuteCount = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type: Qute


$0.it("allows you to register a custom filter which accepts several arguments") {
let template = Template(templateString: "{{ name|repeat:'value\"1\"',\"value'2'\",'(key, value)' }}")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Theres lots of trailing whitespace here, could you please remove / configure editor not to put these here?

@ilyapuchka
Copy link
Collaborator Author

@kylef done

@ilyapuchka
Copy link
Collaborator Author

@kylef can you please take another looks at this pr?

@kylef kylef merged commit 5541eae into stencilproject:master Apr 18, 2017
@kylef
Copy link
Collaborator

kylef commented Apr 18, 2017

@ilyapuchka Yes, sorry for delay in getting back to you on some of these PRs. Struggling to find time lately for OSS.

I really appreciate all the time and effort you've put in here, I'll make a release shortly with your changes.

kylef added a commit that referenced this pull request Apr 18, 2017
@kylef
Copy link
Collaborator

kylef commented Apr 18, 2017

I'll try tackling #94 tomorrow and then make a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants