Skip to content

Formats Per-Command Help Output#1285

Closed
HassanAbouelela wants to merge 1 commit into
python-discord:mainfrom
HassanAbouelela:fix-help-description-spacing
Closed

Formats Per-Command Help Output#1285
HassanAbouelela wants to merge 1 commit into
python-discord:mainfrom
HassanAbouelela:fix-help-description-spacing

Conversation

@HassanAbouelela
Copy link
Copy Markdown
Contributor

Closes #1232

Description

Modifies the docstring sent for per-command help to remove weird formatting issues mentioned in #1232. Removes newlines that are not used for paragraph breaks, after retrieving the docstring, and lets the embed handle it on the discord side.

The Old and Updated Output for Reference

Old
Updated

Modifies the docstring sent for per-command help to remove weird
formatting issues mentioned in #1232.

Signed-off-by: Hassan Abouelela <47495861+HassanAbouelela@users.noreply.github.com>
@HassanAbouelela HassanAbouelela requested a review from a team as a code owner November 14, 2020 00:01
@HassanAbouelela HassanAbouelela requested review from Den4200 and GhostofGoes and removed request for a team November 14, 2020 00:01
@ghost
Copy link
Copy Markdown

ghost commented Nov 14, 2020

Thank you for contributing to Python Discord!

Please check out the following documents:

@ghost ghost added the needs 2 approvals label Nov 14, 2020
@HassanAbouelela HassanAbouelela added a: frontend Related to output and formatting l: 0 - beginner p: 3 - low Low Priority labels Nov 14, 2020
Copy link
Copy Markdown
Contributor

@Numerlor Numerlor left a comment

Choose a reason for hiding this comment

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

This breaks the help of multiple commands like tempban or doc.
Is this worth taking care of? I don't think there's a proper way for us to differentiate between newlines added for readability in the source and the ones that are also necessary in the help output. That'd mean we'd have to change the docstrings with some special syntax or using line continuation which imo is not worth the tradeoff of complicating how it looks in the source code for the small improvement to what's sent on discord

@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Nov 22, 2020
@HassanAbouelela
Copy link
Copy Markdown
Contributor Author

HassanAbouelela commented Nov 24, 2020

I tested this with all the commands I could find in help, and I got the following:

Broken:

  • tempban
  • tempmute
  • tempvoiceban
  • superstarify

Improved:

  • eval
  • close

I'm not actually seeing any difference in doc, so I won't address it. All the other broken commands have their doc syntax in common. I'm seeing a few possible solutions:

  1. Modify the doc of eval and close.
  2. Modify the docs of the broken commands.
  3. Add a check for the doc syntax of the broken commands. (I've prepared an implementation for this if this approach is of interest.)
  4. Ignore and close the issue.

@Numerlor
Copy link
Copy Markdown
Contributor

I'm not actually seeing any difference in doc, so I won't address it. All the other broken commands have their doc syntax in common. I'm seeing a few possible solutions:

  1. Modify the doc of eval and close.
  2. Modify the docs of the broken commands.
  3. Add a check for the doc syntax of the broken commands. (I've prepared an implementation for this if this approach is of interest.)
  4. Ignore and close the issue.

Sorry, it's the get subcommand of doc. Doing the checks at runtime doesn't sound like it's worth it to me, as new commands that may use the newlines in a way that shouldn't be stripped most probably won't be affected by it.
If only the two commands you mentioned get an improvement the manual solution wouldn't be that bad as the amount of affected commands and thus their docstring is that small

@HassanAbouelela
Copy link
Copy Markdown
Contributor Author

Superstarify needs to be fixed too.

@HassanAbouelela
Copy link
Copy Markdown
Contributor Author

As a bit of an update, there is no real good way to fix the output, without changing the docstrings. Paragraphs that pass the 80 character limit have to be spaced onto multiple lines for linting, but discord embeds can't really automatically handle that. There is no good way to stay under the limit, and present a good output.

This means we will have to shorten or break up the broken doc strings or implement a parsing solution.

@Xithrius Xithrius added t: enhancement Changes or improvements to existing features s: stalled Something is blocking further progress help wanted Extra attention is needed and removed l: 0 - beginner s: waiting for author Waiting for author to address a review or respond to a comment labels Feb 25, 2021
Base automatically changed from master to main March 13, 2021 19:40
@jb3 jb3 requested a review from Akarys42 as a code owner March 13, 2021 19:40
@ChrisLovering
Copy link
Copy Markdown
Member

Do we just go for the easy, but tedious, solution and update the doc strings? I think the other options are complex and would add maintenance burden.

We could then just ensure we test what the help embed looks like for future commands before approving.

@Xithrius
Copy link
Copy Markdown
Contributor

Xithrius commented Aug 6, 2021

Do we just go for the easy, but tedious, solution and update the doc strings?

@ChrisLovering this is probably the best way to do it for now.

@Xithrius
Copy link
Copy Markdown
Contributor

@HassanAbouelela we should probably open up another PR to fix the necessary docstrings.

@Xithrius Xithrius added a: docs Adds or updates documentation and removed help wanted Extra attention is needed s: stalled Something is blocking further progress labels Aug 16, 2021
@Xithrius Xithrius added the s: planning Discussing details label Aug 16, 2021
@HassanAbouelela
Copy link
Copy Markdown
Contributor Author

Just to clarify, which solution is this?

There are two similar solutions. The first one is the change added by this PR + changing the doc strings of time units.

The other is to change every single docstring that takes more than one line.

@Xithrius
Copy link
Copy Markdown
Contributor

I think we're going for the 1st option. @ChrisLovering, opinions?

@ChrisLovering
Copy link
Copy Markdown
Member

Yea, the former. We should have a mechanism to force a line break when needed though, for when we need to do things like the time units in future.

@HassanAbouelela
Copy link
Copy Markdown
Contributor Author

I had actually written a small patch to allow for forcefully breaking lines (more specifically, I used the escape sequence used for time units -- \u2003 -- as a non-escapable break).

The implementation had the added benefit of not requiring any docstring changes. At this point, it's gone, and this PR is very outaded, but it should be pretty easy to take the work already done here, add on the patch I just described in a new PR.

@Xithrius Xithrius added p: 2 - normal Normal Priority and removed s: planning Discussing details p: 3 - low Low Priority labels Aug 30, 2021
@Xithrius
Copy link
Copy Markdown
Contributor

@HassanAbouelela I think we should make a new PR for the docstrings if we are to manually format them.

@Xithrius Xithrius added the s: waiting for author Waiting for author to address a review or respond to a comment label Oct 12, 2021
@ChrisLovering
Copy link
Copy Markdown
Member

This has been taken over by #1876

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: docs Adds or updates documentation a: frontend Related to output and formatting p: 2 - normal Normal Priority s: waiting for author Waiting for author to address a review or respond to a comment t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Help command for eval contains unnecessary newlines.

4 participants