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

Fix DataOutputAgent so that it can output items with multiple categories #2110

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

knu
Copy link
Member

@knu knu commented Sep 4, 2017

The to_xml method encodes { "category": ["a", "b"] } as follows:

<item>
 <category>
  <category>a</category>
  <category>b</category>
 </category>
</item>

Instead of this:

<item>
 <category>a</category>
 <category>b</category>
</item>

Even if category is a singular noun. This feature prevents DataOutputAgent from emitting multiple <category> elements (or <enclosure>, etc.) properly, so I've added a tweak to fix the resulted XML document.

I know the code in the current form is far from optimal, so I think we'll have to revisit here soon or later...

@knu knu requested a review from dsander September 4, 2017 08:56
The to_xml method encodes `{ "category": ["a", "b"] }` as follows:
```xml
<item>
 <category>
  <category>a</category>
  <category>b</category>
 </category>
</item>
```
Instead of this:
```xml
<item>
 <category>a</category>
 <category>b</category>
</item>
```
Even if `category` is a singular noun.  This feature prevents
DataOutputAgent from emitting multiple `<category>` elements (or
`<enclosure>`, etc.) properly, so I've added a tweak to fix the
resulted XML document.

I know the code in the current form is far from optimal, so I think
we'll have to revisit here soon or later...
@knu knu force-pushed the output_rss_items_with_multiple_categories branch from c80b0fc to ef03f9f Compare September 4, 2017 11:01
Copy link
Collaborator

@dsander dsander left a comment

Choose a reason for hiding this comment

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

That makes total sense, I think we have a few related issues/feature requests in the past.

Turns out one can parse XML with regular expressions 😉 The worst that could happen is that the regex is not matching and the XML is not transformed as expected, right? If you see any potential that the gsub chain could raise an exception it might be worth rescuing it and returning the XML generated by to_xml.

@knu
Copy link
Member Author

knu commented Sep 5, 2017

Thanks for the review. These gsub calls should be safe and would never raise exceptions.

@knu knu merged commit 2b79fee into master Sep 5, 2017
@knu knu deleted the output_rss_items_with_multiple_categories branch September 5, 2017 13:54
@knu
Copy link
Member Author

knu commented Sep 5, 2017

I think we should implement a new XML generator that generates a DOM tree you can manipulate before serializing to the final string form. The to_xml method is too wild to handle.

@dsander
Copy link
Collaborator

dsander commented Sep 7, 2017

That sounds like a good idea but also some work 😄 This could also allow us to add special tags to embed HTML/XML in CTAGS

@mightymt
Copy link

mightymt commented Dec 3, 2017

I have a question about the current implementation here. As far as I can see, arrays are only handled as described above if they are defined in the agent configuration at design time.
However when an array is passed from an incoming event it is always implicitly joined within a single "category" element instead of there being multiple "category" elements.

Is this by design or am I doing something wrong?

@dsander
Copy link
Collaborator

dsander commented Dec 3, 2017

@mightymt Are you accessing the array with the as_object filter like in the spec here? If you are using {{ some_key }} liquid will join the array into a string.

@mightymt
Copy link

mightymt commented Dec 4, 2017

@dsander Thanks for your reply, that's what I was looking for! Didn't know about this filter.

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.

3 participants