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

Render JSON Feed entries’ content with render_content #149

Merged
merged 1 commit into from
Jul 16, 2018
Merged

Render JSON Feed entries’ content with render_content #149

merged 1 commit into from
Jul 16, 2018

Conversation

bdesham
Copy link
Contributor

@bdesham bdesham commented May 19, 2018

See #148.

This isn’t ready yet; I’m only opening a PR so that I can see the CI output.

@bdesham
Copy link
Contributor Author

bdesham commented Jul 11, 2018

@snarfed I fixed about half of the tests by just updating them with the app’s new behavior. There are some awkward cases left, though. Consider this error from the test_jsonfeed_to_activity_note test:

- A note. link too
+ A note. link too<p><img src="http://example.com/blog-post-123/image"/></p>

Expected value:
{'author': {'displayName': 'Ryan Barrett',
            'image': [{'url': 'http://example.com/ryan/image'}]},
 'content': 'A note. link too',
 'id': 'tag:example.com,2011:blog-post-123',
 'image': [{'url': 'http://example.com/blog-post-123/image'}],
 'objectType': 'note',
 'published': '2012-02-22T20:26:41',
 'summary': 'too cool to summarize',
 'updated': '2013-10-25T10:31:30+00:00',
 'url': 'http://example.com/blog-post-123'}
Actual value:
{'author': {'displayName': 'Ryan Barrett',
            'image': [{'url': 'http://example.com/ryan/image'}]},
 'content': 'A note. link too<p><img '
            'src="http://example.com/blog-post-123/image"/></p>',
 'id': 'tag:example.com,2011:blog-post-123',
 'image': [{'url': 'http://example.com/blog-post-123/image'}],
 'objectType': 'note',
 'published': '2012-02-22T20:26:41',
 'summary': 'too cool to summarize',
 'updated': '2013-10-25T10:31:30+00:00',
 'url': 'http://example.com/blog-post-123'}

The Activity Streams object ends up with an img tag in the content field that is redundant with the image field.

In general, given that JSON Feed needs some information (like embedded image URLs) to be included inline in the HTML while other formats carry that information out-of-band, there is going to be awkwardness converting between different formats, and I think it would be get a lot of work to get a transformation like “convert from AS to JSON Feed and then back to AS” to be idempotent, even assuming that the input is in exactly the format that Granary would have emitted. How would you like me to handle this?

@snarfed
Copy link
Owner

snarfed commented Jul 12, 2018

hey ben, i'm glad you're getting back to this! thank you!

I think it would be get a lot of work to get a transformation like “convert from AS to JSON Feed and then back to AS” to be idempotent

absolutely right, i agree. i don't actually expect all round trip conversions like that to be idempotent. most of the file sets in testdata/ only have some formats (by extension), not all, and some have special extensions like eg .as-from-feed.json, which test_testdata.py special cases to test only a one way conversion. feel free to use that liberally, and/or rename existing files to it to prevent them from testing a round trip.

also, if the test data approach is just too awkward altogether for testing something specific, feel free to add code-based tests to test_jsonfeed.py instead, or in extreme cases, even move testdata files there. that should hopefully be unlikely though.

thanks again!

This means that Instagram username and hashtags, etc. will now be
hyperlinked in JSON Feeds the same way they are in Atom feeds.
@bdesham bdesham changed the title WIP: Render JSON Feed entries' content with render_content Render JSON Feed entries’ content with render_content Jul 15, 2018
@bdesham
Copy link
Contributor Author

bdesham commented Jul 15, 2018

This is ready to be reviewed! I changed the tests in a way that made sense to me, but let me know what you think.

@snarfed
Copy link
Owner

snarfed commented Jul 16, 2018

looks great. thank you again for sticking with this!

@snarfed snarfed merged commit 076541e into snarfed:master Jul 16, 2018
@snarfed
Copy link
Owner

snarfed commented Jul 16, 2018

deployed!

@bdesham
Copy link
Contributor Author

bdesham commented Jul 16, 2018

Awesome! Glad I could help 😃

@bdesham bdesham deleted the render-json-feed-content branch July 23, 2018 22:09
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.

2 participants