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

Handle long help post titles gracefully #163

Merged
merged 9 commits into from
Feb 25, 2025

Conversation

00-kat
Copy link
Contributor

@00-kat 00-kat commented Feb 24, 2025

Continuation of #151, thank you GitHub.

Before this change, if the new title was longer than 100 characters, an exception would be raised.

This is very hacky and probably the worst code I've ever PR'd. Perhaps "Unable to change the post title." should be returned without all that effort to show the error from the Discord API; I can change the PR to do so if requested, though I would prefer to have the nicer message so the user of the command actually knows what happened.

@trag1c trag1c self-requested a review February 24, 2025 23:47
@00-kat 00-kat force-pushed the handle-long-post-title branch 3 times, most recently from b8b79cd to 29ab437 Compare February 25, 2025 07:50
@00-kat 00-kat requested a review from trag1c February 25, 2025 07:54
00-kat and others added 6 commits February 25, 2025 18:57
Before this change, if the new title was longer than 100 characters, an
exception would be raised.
If there are random square brackets lying around that also get stripped
by this, there are probably bigger issues to deal with.

Co-authored-by: trag1c <[email protected]>
As well as needless duplication of identical format strings.
This both removes the large comment as the format is in the pattern
itself, and makes the code much more readable.

I had completely forgotten that Python had pattern matching, oops!
Thanks trag1c.

Co-authored-by: trag1c <[email protected]>
@00-kat 00-kat force-pushed the handle-long-post-title branch 4 times, most recently from c35f8eb to 91fe8e9 Compare February 25, 2025 08:07
A quick summary:
  - Remove superfluous pattern matching and just directly access the
    field.
  - Avoid blind attempts to make the error message grammatically
    correct, and instead simply rethrow the error if we didn't expect
    it, and hard-code an error message for the errors we expected.
This way it stands out from the original additional reply.
@00-kat 00-kat force-pushed the handle-long-post-title branch from 91fe8e9 to 55ac713 Compare February 25, 2025 08:09
Copy link
Member

@trag1c trag1c left a comment

Choose a reason for hiding this comment

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

One nit, LGTM other than that.

@00-kat 00-kat force-pushed the handle-long-post-title branch from 543b4c5 to f10bb51 Compare February 25, 2025 08:32
@00-kat 00-kat requested a review from trag1c February 25, 2025 08:32
@trag1c trag1c merged commit b6d6a79 into ghostty-org:main Feb 25, 2025
1 check passed
@00-kat 00-kat deleted the handle-long-post-title branch February 25, 2025 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants