Skip to content

adds support for selftext in link, image, gallery, and video posts #2067

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aaronjbecker
Copy link

@aaronjbecker aaronjbecker commented May 6, 2025

Fixes #1965 link

Implement's Reddit's new(ish) support for text in all post types.

Feature Summary and Justification

  • subreddit.submit() will now pass a non-empty selftext string to the API alongside url to provide body text for a link submission.
  • Image, video, and gallery posts now support selftext, which is passed as the text API parameter

Caveat/limitation (noted in doc strings and tested for): I'm not sure whether body text for a link submission supports inline media so I'm throwing a NotImplementedError if a user tries to submit link body text with inline media. Since there's no inline media argument for image, video, or gallery post types I'm not performing this check.

References

Builds on a previous pull request by DevGW

pre_push.py passes.

I still need to add integration tests and need some guidance on how to add them. I'm not familiar with the method that's being used to mock (or record?) API requests.

data.update(kind="link", url=url)
if inline_media:
err_msg = "Link submission support for inline media in body text is unknown and unimplemented."
raise NotImplementedError(err_msg)
Copy link
Member

Choose a reason for hiding this comment

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

This can be verified with an integration test.

@LilSpazJoekp
Copy link
Member

It's a great start! However, we need a few more things:

  1. We need some integration tests added.
  2. A change log entry.
  3. Optionally, add yourself to the contributors in the AUTHORS.rst file.

@aaronjbecker
Copy link
Author

@LilSpazJoekp regarding the integration tests, just to confirm, this is using the betamax library (are these the right docs?)?

Is the general workflow something like recording integration tests against the actual API using my personal account and then committing redacted test data? I'll read the docs, just need some guidance on where to start

@LilSpazJoekp
Copy link
Member

Essentially, yes. The test suite automatically redacts sensitive information.

I would start here: https://praw.readthedocs.io/en/latest/package_info/contributing.html

@aaronjbecker
Copy link
Author

@LilSpazJoekp I've confirmed via integration test that the reddit API doesn't support inline media for the optional body text applied to non-self posts:

def test_submit__url_selftext_inline_media(self, image_path, reddit):
        reddit.read_only = False
        subreddit = reddit.subreddit(pytest.placeholders.test_subreddit)
        gif = InlineGif(caption="optional caption", path=image_path("test.gif"))
        image = InlineImage(caption="optional caption", path=image_path("test.png"))
        video = InlineVideo(caption="optional caption", path=image_path("test.mp4"))
        url = "https://praw.readthedocs.org/en/stable/"
        selftext = (
            "Text with a gif {gif1} an image {image1} and a video {video1} inline"
        )
        media = {"gif1": gif, "image1": image, "video1": video}
        submission = subreddit.submit("title", url=url, inline_media=media, selftext=selftext)
        assert submission.author == pytest.placeholders.username
        # inline media is NOT supported for link submission optional body text
        assert submission.selftext == ''
        assert submission.title == "title"

I want to raise an exception when a user tries to submit inline media along with a link post, which would render this test failing. The other post types don't have an inline media option anyway (since there was no selftext to add inline media to) so there's no need to add a new check to image/gallery/video.

My current plan was to add a unit test that expects an exception when trying to use inline media with a URL post, and remove this integration test, since I think it makes sense to raise an exception instead of making an API request that we know won't work. Does that make sense to you?

@LilSpazJoekp
Copy link
Member

I want to raise an exception when a user tries to submit inline media along with a link post, which would render this test failing.

I would prefer not raising an exception when a user tries to use inline media. If the API throws an exception, we can raise that but PRAW typically doesn't validate requests before sending them to Reddit. Instead, I would add a warning in the description for the inline media parameter in the docstring (and maybe throw a warning) that says something along the lines of: "As of 2025-05-07, inline media in the selftext of link posts is not supported."

My current plan was to add a unit test that expects an exception when trying to use inline media with a URL post, and remove this integration test, since I think it makes sense to raise an exception instead of making an API request that we know won't work. Does that make sense to you?

Makes sense, however, I would consider a different approach. As I said above, PRAW doesn't typically validate requests before calling the API. This is due to how often Reddit changes the API. For instance, adding selftext to all post types! There is validation that throws a type error. Now they've added support for it and it now requires a code change to support the new feature. They could add support inline media to selftext on the other post types in the future.

Does the API throw an exception? If so, we can use an integration test and make sure that exception is thrown. If not, then we could validate the selftext on the created submission and see if it contains the inline media. In this case, I would be good with throwing a warning before sending the request or checking the selftext afterwards and throw a warning if it wasn't added.

That being said, I see an argument to throw an exception before calling the API if an exception isn't thrown by Reddit since it involves creating a post–it wouldn't be the best experience if PRAW allows users to create broken posts.

@aaronjbecker
Copy link
Author

The Reddit API doesn't throw an exception when you try to use richtext_json with a non-self post, it just silently fails and submission.selftext is an empty string. I think throwing an exception here offers better UX because creating a post that doesn't conform to your expectations is very inconvenient.

Also, Reddit seems unlikely to add support for inline media to text for non-self post types... the use case proposed in the reddit announcement regarding adding text to all post types is short comments that accompany links or media. Adding inline media to a media post (image/video/gallery) seems counter-intuitive.

I also don't like that letting the post go ahead involves explicitly adding and leaving code in that we know doesn't work:

        if url is not None:
            data.update(kind="link", url=url)
            if inline_media:
                # this will never work: reddit API ignores richtext_json for link posts
                # so throw an exception here instead of allowing silent failure?
                body = selftext.format(**{
                    placeholder: self._upload_inline_media(media) for placeholder, media in inline_media.items()
                })
                converted = self._convert_to_fancypants(body)
                data.update(richtext_json=dumps(converted))
            # we can ignore an empty string for selftext here b/c body text is optional for link posts
            elif selftext:
                data.update(text=selftext)
        elif selftext is not None:
            data.update(kind="self")
            if inline_media:
                body = selftext.format(**{
                    placeholder: self._upload_inline_media(media) for placeholder, media in inline_media.items()
                })
                converted = self._convert_to_fancypants(body)
                data.update(richtext_json=dumps(converted))
            else:
                data.update(text=selftext)

Unless you have strong objections I'm going to move forward with throwing an exception, adding a unit test, and removing the soon-to-be-broken integration test.

@LilSpazJoekp
Copy link
Member

Makes sense to me! No objections here!

@aaronjbecker
Copy link
Author

@LilSpazJoekp I'm having trouble with the integration test for the video post with selftext. I keep getting this error:

FAILED tests/integration/models/reddit/test_subreddit.py::TestSubreddit::test_submit_video__selftext - praw.exceptions.WebSocketException: Websocket error. Check your media file. Your post may still have been created.

How can I mock the websocket when I don't know the post ID until the cassette runs?

When I look at the cassette that's recorded I'm seeing something extremely strange-- the POST to reddit-uploaded-media:

        "method": "POST",
        "uri": "https://reddit-uploaded-media.s3-accelerate.amazonaws.com/"

Seems to have the text of the subreddit.py in the request body (???):

\nContent-Disposition: form-data; name=\"Content-Type\"\r\n\r\nimage/jpeg\r\n--a4db0f7e5b73f8ea5f79c1fea6abc11a\r\nContent-Disposition: form-data; name=\"file\"; filename=\"subreddit.py\"\r\n\r\n\"\"\"Provide the Subreddit class.\"\"\"\n\nfrom __future__ import annotations\n\nimport contextlib\nfrom copy import 

Do you have any idea what could be causing something like this? The other integration tests have all worked without issue but I'm stuck on the video post.

@LilSpazJoekp
Copy link
Member

That is extremely strange! Could you set a breakpoint on the call to reddit-uploaded-media? Also what does the test look like?

@LilSpazJoekp
Copy link
Member

How can I mock the websocket when I don't know the post ID until the cassette runs?

I missed this. You'll need to comment out the mock run the test and then grab the id from the cassette.

@aaronjbecker
Copy link
Author

I found the error! It's at the start of upload_media, which gets called by submit_video when there's no thumbnail provided. When there's no thumbnail the current code uploads subreddit.py!

the offending part of submit_video:

        data.update(
            kind="videogif" if videogif else "video",
            url=video_url,
            # if thumbnail_path is None, it uploads the PRAW logo
            video_poster_url=self._upload_media(media_path=thumbnail_path),
        )

Which is meant to be caught here at the start of _upload_media but since only file gets used downstream that's what gets uploaded!

        if media_path is None:
            file = Path(__file__).absolute()
            media_path = file.parent.parent.parent / "images" / "PRAW logo.png"
        else:
            file = Path(media_path)

I'm going to change the submit_video code to *not upload any thumbnail if none is provided. I don't know what implications this will have for other unit or integration tests... some may need to be re-run with new cassettes.

To me even the intended behavior, uploading the PRAW logo as a default video_poster_url, doesn't make sense (unless a thumbnail is required by the API?). The video_poster_url is not specified anywhere in the integration test suite...

@LilSpazJoekp
Copy link
Member

I'm pretty sure it's required by the API. It's not specified in the test since a default one is used.

@aaronjbecker
Copy link
Author

OK if video_poster_url is required then I'll just fix the check at the start of _upload_media to upload the PRAW logo as intended. The integration tests for video submission use cassettes that were all recorded ~4Y ago so that might have been a regression that happened since then (git blame shows changes ~2Y ago).

@aaronjbecker
Copy link
Author

@LilSpazJoekp I just pushed a few more commits with the integration tests; a couple of them seem to be failing in CI so I'll dig into that in the AM.

As for the change log-- do I just add the changes to the "Unreleased" section in CHANGES.rst?

@LilSpazJoekp
Copy link
Member

That's correct!

@aaronjbecker
Copy link
Author

@LilSpazJoekp All of the CI tests now report passing and I updated the change log.

FYI I was encountering a weird behavior in the CI tests where instances of the word "test" in selftext or comment fields were being replaced by the placeholder <TEST_SUBREDDIT> or placeholder_test_subreddit when the tests were run on the CI server. Not sure what the deal was there but the workaround was just to modify the recorded tests, left a note in one of the impacted tests.

Should I squash all of these commits into a single one or hold off until you review to see if any other changes are required?

@aaronjbecker aaronjbecker requested a review from LilSpazJoekp May 8, 2025 16:49
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.

Support adding text on all post types
2 participants