Errors on setting class via dict

[metasoarous]

When I try to render something simple like ["span", {"class": "foo"}, "hello"], it errors with AttributeError: 'str' object has no attribute 'extend'. The same class assignment works perfectly via the "span.foo" syntax, but this is rather annoying to have this edge case.

[nosamanuel]

Thanks for the report! It looks like the assumption I originally made was that the explicit class defined by tag.className would be an invariant, and class would be an optional list of additional classnames passed in the the attributes that would be appended to the invariant classnames.

See https://github.com/nosamanuel/cottonmouth/blame/master/cottonmouth/html.py#L87.

I think this violates the principle of least surprise though so it's probably worth treating this as a bug and not a feature :)

There is still a design question to answer though. In the case where classnames are set by both the tag and the attributes, should we:

1. Set the classnames to the union of both by joining the strings?
2. Silently override any class names set in the tag name?
3. Raise a ValueError?

I'm inclined toward option 3 since the first two are only alternative implementations to the questionable code that caused this bug in the first place.

[nosamanuel]

I guess there's an option #4 too, where we accept both strings or lists for classnames but that seems kind of gross because then we'd need to apply the same implementation to all HTML attributes that accept multiple space-separated string values.

I added a pull request that can move forward if we decide on option #3.

[metasoarous]

I see. I'd be inclined to leave the current behavior, now that I understand it. Passing a list is nice because it's easier to compose than munging strings together. And composition is the name of the game with (and the whole point of) hiccup. I think either a note in the docs or allowing both string and list class arguments would be fine. And I'd vote for merging class names given static tag classes and a class list.

[nosamanuel]

Hmm... so maybe extend the permissibility to both iterables and strings? That's more than Hiccup does but for many cases I do think lists are more natural data structures for dynamic class names!

Comparison:

=> (use 'hiccup.core)
=> (html [:span.foo {:class "bar"} "hello"])
"<span class=\"foo bar\">hello</span>"
=> (html [:span.foo {:class ["bar"]} "hello"])
"<span class=\"foo [&quot;bar&quot;]\">hello</span>"

[nosamanuel]

I've updated the PR to support (and document) both styles. Code review much appreciated.

[metasoarous]

Looks good to me!

[nosamanuel]

@metasoarous published v0.2.2 to PyPI with the changes, thanks again for contributing at this weird intersection of Python and Clojure!