Skip to content

Limit issue body to 500 characters #488

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

Merged
merged 2 commits into from
Oct 7, 2020
Merged

Limit issue body to 500 characters #488

merged 2 commits into from
Oct 7, 2020

Conversation

quanta-kt
Copy link
Contributor

Relevant Issues

Closes #486

Description

This PR puts a limit on how long the response sent by the .hacktoberissues is going to be by limiting the issue body up to it's first 500 characters. This avoids http 400 errors (see #486 ) when dealing with issues with very long body.

Did you:

  • Join the Python Discord Community?
  • If dependencies have been added or updated, run pipenv lock?
  • Lint your code (pipenv run lint)?
  • Set the PR to allow edits from contributors?

@quanta-kt quanta-kt requested a review from a team as a code owner October 6, 2020 15:18
@quanta-kt quanta-kt requested review from dementati and Den4200 October 6, 2020 15:18
@ghost ghost added the needs 2 approvals label Oct 6, 2020
@ghost
Copy link

ghost commented Oct 6, 2020

Thank you for contributing to Python Discord!

Please check out the following documents:

Copy link
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the contribution!

@@ -103,7 +103,7 @@ def format_embed(issue: Dict) -> discord.Embed:
labels = [label["name"] for label in issue["labels"]]

embed = discord.Embed(title=title)
embed.description = body
embed.description = body[:500] + '....' if len(body) > 500 else body
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m being very picky, but would it be possible to use only 3 dots here? That’s what we usually use.

Suggested change
embed.description = body[:500] + '....' if len(body) > 500 else body
embed.description = body[:500] + '...' if len(body) > 500 else body

Copy link
Member

Choose a reason for hiding this comment

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

You should just use textwrap.shorten instead.

Copy link
Contributor

@kwzrd kwzrd left a comment

Choose a reason for hiding this comment

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

What is the motivation for 500? The issue discusses that the embed description can be up to 2048 characters in length.

@quanta-kt
Copy link
Contributor Author

@kwzrd 500 is just a short number which I think is decent amount characters for embed descriptions. I agree this is opinionated, (large embeds just don't look good to me/I feel it's bad UX). So being pointed out, I'll limit it back to 2048 characters if that's agreeable.

@kwzrd
Copy link
Contributor

kwzrd commented Oct 6, 2020

It's not necessarily a bad choice, if you think that 500 looks and works better. I was just wondering about why it was chosen. Maybe you could show a screenshot of 500 vs 2048?

@quanta-kt
Copy link
Contributor Author

@kwzrd Here are the screenshots you requested-
https://ibb.co/M21rqX2
https://ibb.co/gDM642P
The one with 2048 characters totally covers my laptop screen.

@kwzrd
Copy link
Contributor

kwzrd commented Oct 6, 2020

@quanta-kt Thanks for the examples, I agree that 500 is a more sane value, especially since you can just click the link to read more.


@Den4200 @Akarys42 Are you sure that textwrap.shorten is the better choice here? The problem with it is that it removes newlines. Consider this issue, which gets formatted like so:

image

If we just slice the string, we get to retain the newlines, e.g. this issue:

image

@Den4200
Copy link
Member

Den4200 commented Oct 6, 2020 via email

@quanta-kt
Copy link
Contributor Author

TextWrapper apparently has a replace_whitespace keyword-arg but shorten simply ignores it, weird.

Copy link
Contributor

@kwzrd kwzrd left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, it works great!

Before we merge this, we would be interested in cleaning up the history because there are 5 commits for a 1-line change. In practice, what we would want to do is only keep the first 2 commits, and drop the last 3. This is not your fault, as we told you to make a change and then asked you to revert it.

Are you familiar with git rebase and would you feel confident trying to do this? If not, that's ok, a core dev can help you or do it for you.

@ghost ghost removed the needs 1 approval label Oct 7, 2020
@quanta-kt
Copy link
Contributor Author

quanta-kt commented Oct 7, 2020

Yes I am familiar with rebasing but still a little hesitant to do it, can you check this dummy PR here and confirm if that is how it should be looking like?
Commands used-

git rebase -i issue-486~5 issue-486 # And picking only the first commit
git push origin +issue-486 # A forced push

Also, shouldn't it get squashed into a single commit automatically if one chooses to "Squash and merge" on GitHub?

@kwzrd
Copy link
Contributor

kwzrd commented Oct 7, 2020

Your dummy PR looks good, but you would want to keep the 2nd commit as well (reducing the dots from 4 to 3).

I think there is an easier way, we actually don't need to rebase because we only need to drop the last 3 commits:

git reset --hard HEAD~3

You can use git log to verify that only commits 5f02c61 and 8645da6 remain. Then git status should say that your local branch is 3 commits behind the remote. Finally git push --force will remove the commits from the remote as well.

Also, shouldn't it get squashed into a single commit automatically if one chooses to "Squash and merge" on GitHub?

Yeah, we had a chat internally and we wanted to give you a chance to do it as it's both a good learning opportunity and also makes sure that the actual commits that get merged are authored by you, rather than the person who squashed them.

@kwzrd
Copy link
Contributor

kwzrd commented Oct 7, 2020

Lovely, thank you!

@kwzrd kwzrd merged commit c56cf1e into python-discord:master Oct 7, 2020
@quanta-kt quanta-kt deleted the issue-486 branch October 7, 2020 13:27
@kwzrd kwzrd added area: frontend Related to output and formatting hacktoberfest-accepted type: bug Something isn't working labels Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: frontend Related to output and formatting type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP 400 in command hacktoberissues when the issue description is too long
4 participants