Support nested attribute hashes rendered as hyphenated attributes

[Ikariusrb]

Closes #450

[javierjulio]

This looks great, thanks! Only a minor note on the context grouping since I figured what's here was unintentional.

Would you mind adding another context with the same test case pairing but testing that a data attribute as a full named key (with hyphens, not using nested hashes) works as expected?

[Ikariusrb]

Would you mind adding another context with the same test case pairing but testing that a data attribute as a full named key (with hyphens, not using nested hashes) works as expected?

I'm not quite certain what you're asking for here. You're asking for a test showing that the (exceedingly awkward) code that I (ab)used to get Arbre to emit a hyphenated attribute before still works? If that's what you're asking for, I can add a test easily, but IMO the code I used to do that was terrible and should never be encouraged, and adding a test to codify it as part of the developer contract would be encouragement in my eyes.

If that's not what you're asking for, I'm a bit lost, and require further clarification :)

[javierjulio]

@Ikariusrb no but I think we should still address the original issue. I'm good with the support here for nested hashes, so thank you! My understanding is that with your changes, it should resolve the original issue you had right? For example, this test case passes with your changes.

      it "should render the attributes to html" do
        tag.build id: "my_id", "data-action" => "action"
        expect(tag.to_s).to eq "<tag id=\"my_id\" data-action=\"action\"></tag>\n"
      end

I was thinking for completion it would be nice to have a set of tests like this in the pattern you set up, to further demonstrate that hyphenated keys rather than nested hashes also works as expected now. What do you think?

[Ikariusrb]

@javierjulio
My PR doesn't change the ability to pass in hyphenated attributes directly at all. The problem is ruby itself doesn't support hyphenated symbols constructed with colon shorthand syntax, and Arbre expects to receive the attributes as keyword args, so you have to construct a hash, use .to_sym and rocket to get a hyphenated key, then double-splat it out to the keyword args to get it passed in as Arbre expects. I consider that workaround to've been mostly a hack.

Changing the call signature for the tag builders is something I'd rather not attempt to touch, as that's far more likely to break something than what I added. The approach would likely be to accept attributes as a hash. I believe ruby will gather keyword args and put them in a hash if there's a final empty positional arg in a method signature, but to make that change I'd want to review the tests with more care than I have time for to make sure all cases were covered, and even then I'd be worried someone would've done something fun and unique in the wild that would end up breaking.

[Ikariusrb]

@javierjulio - any update on the ability for this to be merged?

[javierjulio]

@Ikariusrb yes, I will get to this soon, thank you.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.12 🎉

Comparison is base (b09b813) 91.23% compared to head (faf4f99) 91.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
+ Coverage   91.23%   91.36%   +0.12%     
==========================================
  Files          17       17              
  Lines         468      475       +7     
==========================================
+ Hits          427      434       +7     
  Misses         41       41              

Impacted Files	Coverage Δ
lib/arbre/html/attributes.rb	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[javierjulio]

@Ikariusrb I rebased to get CI green again on the latest. I reviewed this and realize why this makes no changes to hyphenated attributes. I remember now why I asked though since I wanted to have a test case to demonstrate that it works as we didn't have a test for that. The reason why is that the arguments are forwarded and the last arg is a hash, so keyword arguments in this case should not matter. It wasn't dependent on your change. It's a related request to improve the tests.

[javierjulio]

Thanks!