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

Improve clean_content rationale and documentation #1911

Closed
scarletcafe opened this issue Feb 20, 2019 · 1 comment
Closed

Improve clean_content rationale and documentation #1911

scarletcafe opened this issue Feb 20, 2019 · 1 comment
Labels
docs needed Documentation is needed for this change. v1.0-alpha This pertains to the rewrite version

Comments

@scarletcafe
Copy link
Contributor

While Message.clean_content is often used and recommended by people when they need message input "sanitized", I find that a lot of the time, very little thought is put into what this actually means.

As I see it, most of the time, when someone needs their content "clean", they mean one of a few things:

  • Content that has had its context filled in, with user/channel/role mentions resolved to their real names and not much else (suitable for e.g. logging in text files),
  • Content similar to as if one had clicked and dragged with their mouse over the message and copied it, such that mentions are resolved to their real name and markdown is removed, leaving only the pure text content,
  • Content that has no "side effects" when sent back to Discord, with user and role mentions being escaped (but not channel mentions), and @everyone and @here pings escaped (suitable for e.g. reminders, tagging or memos),
  • Content that escapes in such a way that when sent to Discord it accurately represents what would have been typed in the message to produce such an output (suitable for e.g. 'raw' commands)

Despite the wide array of interpretations, the documentation as of current commit doesn't really help narrow down what the property is aiming for:

def clean_content(self):
"""A property that returns the content in a "cleaned up"
manner. This basically means that mentions are transformed
into the way the client shows it. e.g. ``<#id>`` will transform
into ``#name``.
This will also transform @everyone and @here mentions into
non-mentions.
"""

A "cleaned up" manner really doesn't describe anything at all. Then it says it's as the client shows it, so maybe only text content, with no markdown? The mentions being transformed could describe any of the first two, but wait, it escapes @everyone, so are we going for no side effects? Hold on, #1885 suggests we're escaping markdown, so are we going for raw instead?

Perhaps clean_content should be split into multiple separate properties, each with separate goals to satisfy each condition? Or, maybe, it should be changed to a cleaning function instead, with arguments that allow the user to adjust exactly what they feel is the correct approach?

Whichever way it ends up going, at the very least, the goal for the property should be clear and the documentation unambiguous, else each user will have no choice but to source dive to figure out exactly what to expect for something that should not really be all that complicated.

@Bluenix2
Copy link
Contributor

It could be separated into clean_markdown() and clean_mentions(). But keep clean_content() to do both.
Another way could be like wait_for() and so you pass in a string either markdown or mentions and so maybe both or all. Which is probably better.

@Rapptz Rapptz added v1.0-alpha This pertains to the rewrite version dep bug A bug with the dependencies docs needed Documentation is needed for this change. and removed dep bug A bug with the dependencies labels Mar 9, 2019
@Rapptz Rapptz closed this as completed in 79a8249 Apr 7, 2019
yagomichalak pushed a commit to yagomichalak/discord.py that referenced this issue May 28, 2021
DiEVeXx pushed a commit to DiEVeXx/discord.py that referenced this issue Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs needed Documentation is needed for this change. v1.0-alpha This pertains to the rewrite version
Projects
None yet
Development

No branches or pull requests

3 participants