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

Code-as-attributes doesn't work, e.g. src=static_path(@conn, "x.css") #32

Closed
henrik opened this issue Sep 8, 2015 · 10 comments · Fixed by #115
Closed

Code-as-attributes doesn't work, e.g. src=static_path(@conn, "x.css") #32

henrik opened this issue Sep 8, 2015 · 10 comments · Fixed by #115
Labels

Comments

@henrik
Copy link
Contributor

henrik commented Sep 8, 2015

This gives a "missing terminator" error:

link rel="stylesheet" href=static_path(@conn, "/css/app.css")

This works, though:

link rel="stylesheet" href="#{static_path(@conn, "/css/app.css")}"

The equivalent of the first example works in the Ruby version of Slim.

Maybe it could make sense to include a valid example of a dynamic attribute value in the README docs?

@doomspork
Copy link
Member

Thanks @henrik, this looks like a bug.

@henrik
Copy link
Contributor Author

henrik commented Sep 9, 2015

Glad to see this library is alive and well and being maintained :) Thanks so much for all the hard work!

Sent from my iPhone

On 09 Sep 2015, at 07:17, Sean notifications@github.com wrote:

Thanks @henrik, this looks like a bug.


Reply to this email directly or view it on GitHub.

@doomspork
Copy link
Member

You're welcome! Myself or @Rakoth will take a look at these bugs. Thanks again for taking the time to file them. 👍

@Rakoth
Copy link
Member

Rakoth commented Sep 9, 2015

Hi @henrik, we have some problems with elixir code with spaces as attribute values.

link rel="stylesheet" href=static_path(@conn,"/css/app.css")

should work for you now

@doomspork
Copy link
Member

This one is going to take some regex magic to sort out. The offending regex at parser.ex#L25, specifically this part: [^\s"'][^\s]+[^\s"']. We need to update the regex to allow spaces within () if present.

@henrik
Copy link
Contributor Author

henrik commented Sep 10, 2015

@Rakoth Thanks for the workaround!

Regarding the regex, I believe checking for balanced parentheses (if we want to allow href=foo(a, b()) but not href=foo(a, b() is one of those things that "pure" regexps can't support. But Erlang/Elixir's flavor seems quite full-featured, so I'm sure there's something in there. More info: http://stackoverflow.com/questions/133601/can-regular-expressions-be-used-to-match-nested-patterns

If all else fails, I guess parse_attributes could do something beyond regexps.

@henrik
Copy link
Contributor Author

henrik commented Sep 10, 2015

This seems applicable: http://www.erlang.org/doc/man/re.html#sect20

No time to dig further into it now I'm afraid, but maybe later if you don't beat me to it (feel free).

@henrik
Copy link
Contributor Author

henrik commented Sep 10, 2015

Experimenting with the linked regexp, I get this:

re = ~r/
^
  \w+
  (
    \(
      ([^()]++|(?1))*
    \)
  )
$
/x

[
  "foo()",
  "foo(bar, baz(a(), 5, :dog, b(c, 123)))",
  "foo('a (')",
  "foo(()",
  "foo(bar, baz()",
] |> Enum.each fn (string) ->
  IO.puts "#{String.match?(string, re)} for #{string}"
end

With output:

true for foo()
true for foo(bar, baz(a(), 5, :dog, b(c, 123)))
false for foo('a (')
false for foo(()
false for foo(bar, baz()

So it handles nested parentheses but parentheses inside a string can mess it up. Handling those (and maybe accounting for escaped quotes inside strings…) could get hairy. I'm thinking if this should be handled perfectly, a regexp probably isn't the way to go.

@dannote
Copy link

dannote commented Aug 14, 2016

@doomspork Is there any progress on this issue?

@doomspork
Copy link
Member

@dannote I don't believe anyone has worked on this since there is a reasonable work-around, though it would be nice to resolve.

bfad added a commit to bfad/slime that referenced this issue Dec 4, 2016
Any attribute that had an Elixir expression with a space in it would not
be parsed correctly. This commit should mitigate most instances.

Fixes slime-lang#32
doomspork pushed a commit that referenced this issue Dec 22, 2016
Any attribute that had an Elixir expression with a space in it would not
be parsed correctly. This commit should mitigate most instances.

Fixes #32
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 a pull request may close this issue.

4 participants