Strings as Nori::StringWithAttributes about nori HOT 8 CLOSED

Thanks for opening this issue @gnilrets. I'm trying to follow along with the discussion from two years ago in the other pull request and issue for this. It seems that @wilg suggested that added a conditional to this code in Nori would be easy, but @rubiii countered by saying that this would break the implicit promise of Nori that all nodes it parses have an #attributes method -- requiring users to check for #responds_to?(:attributes) in a lot of places.

That's kind of an awkward situation. Is it possible the Nori::StringWithAttributes class could just implement a method for YAML dumping that would make this work?

from nori.

@tjarratt - Seems like that would work. I've really only seen this issue when dumping to YAML. I've even used JSON.pretty_generate in other situations without issue.

from nori.

Will need to investigate if it's even possible to implement a method for dumping the string value correct for YAML.

I also noticed that @wilg suggested that this could be fixed by requiring the psych gem. Have you tried that? I can't imagine that would possibly work but it's worth a shot.

from nori.

A fix for this issue would be helpful...to me at least 👍

from nori.

@bobmacneal can you explain why turning a hash of Nori::StringWithAttributes to YAML is meaningful to you? I lack the imagination to see why that would be important, aside from occasional debugging.

It's also unclear to me what could be done to fix this. I don't have the time myself to invest in finding a solution, but I'd be more than happy to merge a pull request that fixes this.

from nori.

I've noticed that this issue no longer appears on ruby 2.2.0.

The reason that we're doing this is because of saving parts of the xml as as erialized attribute in a Rails model which uses YAML dumping. I previously worked around it by monkeypatching Nori to use regular String classes and ignore the attributes since we're not using them anyway. Now with ruby 2.2.0 the dumping works as expected.

from nori.

Thanks for the update @jeroenj! Given that, I'm inclined to close this since there's a path forward to make this work. How do you feel about that @gnilrets, given that you originally opened this issue?

from nori.

That's fine @tjarratt. I'm not using 2.2 yet but I do have a workaround in place now.

from nori.