Fix formatting of code longer than Discord's message limit#549
Fix formatting of code longer than Discord's message limit#549Neil-Tomar wants to merge 1 commit into
Conversation
danthe1st
left a comment
danthe1st
left a comment
There was a problem hiding this comment.
Congratulations on writing your first pull request. I commented on a few things I noticed. Please don't unnecessarily duplicate code.
| * Splits {@link #content} into pieces that each fit within {@link #MAX_SIZE}, | ||
| * breaking on newlines where possible so lines are not cut in half. | ||
| */ | ||
| public List<String> toDiscordChunks() { |
There was a problem hiding this comment.
Is there a reason for this being public?
| * @return the matching language, or {@link #UNKNOWN} if none matches | ||
| */ | ||
| public static Language fromString(String name) { | ||
| for (Language language : values()) { |
There was a problem hiding this comment.
Instead of this loop, you could use something like this:
try {
return Language.valueOf(name.toUpperCase());
} catch (IllegalArgumentException e) {
return UNKNOWN;
}| IndentationHelper.IndentationType.TABS); | ||
|
|
||
| if (indented.isBlank()) { | ||
| event.reply("There is no code to format in that message.").setEphemeral(true).queue(); |
There was a problem hiding this comment.
If you use Responses.error(event, "There is no code to format in that message.").queue();, it would show a nicer error message.
Aside from that, is there any possibility of of the string being blank here?
| } | ||
| channel.sendMessage(messages.get(index)) | ||
| .setAllowedMentions(List.of()) | ||
| .setComponents(FormatCodeCommand.buildActionRow(event.getTarget(), event.getUser().getIdLong())) |
There was a problem hiding this comment.
The action row including the buttons for deleting and the URL for jumping back are no longer present because you removed them here. Please add them to all messages.
There was a problem hiding this comment.
There is a reason for that. The buttons only affect the message they are attached to, so adding them to each message would only allow deletion of that specific message.
Adding buttons to every message would also introduce a visual break inside the code block, which could hurt readability, as shown in the screenshots.
If you'd prefer the buttons to appear only on the last message while still affecting the entire code block, I can look into implementing that. I'm not familiar with that approach yet, but I'm happy to investigate it.
Here is with buttons:
as you can see it's hard to read through indentation.
There was a problem hiding this comment.
If it is a single message, both buttons should definitely be there.
If it consists of multiple messages, at least the View Original button should be present in the last message. There could also be a delete button that deletes all codeblock messages on the last message (and the file upload message should also have both buttons).
| .queue( | ||
| success -> sendChunksInOrder(channel, messages, 0, event), | ||
| error -> { | ||
| ExceptionLogger.capture(error, getClass().getSimpleName()); |
There was a problem hiding this comment.
I personally think it isn't necessary to add error logic like this when it "should never happen". If there is no handler, JDA logs the error either way. However, it's fine if you prefer it that way.
| .addOptions( | ||
| new OptionData(OptionType.STRING, "message-id", "Message to be formatted, last message used if left blank.", false), | ||
| new OptionData(OptionType.STRING, "format", "The language used to format the code, defaults to Java if left blank.", false) | ||
| .addChoice("C", "c") |
There was a problem hiding this comment.
Since you already introduced a Language enum, I think you can iterate over the values there and use that instead of listing them all here. Otherwise, the enum is pretty useless without your follow-up PR.
| } | ||
| } | ||
|
|
||
| private void sendFormattedCode(SlashCommandInteractionEvent event, Message target, String format, String indentation) { |
There was a problem hiding this comment.
There is quite a bit of code duplication between these methods and the ones in the other classes. Please refactor that to only have the common logic once.
| this.content = content; | ||
| } | ||
|
|
||
| public String getContent() { |
There was a problem hiding this comment.
I think getContent() and setLanguage are unused. If so, please remove them.
|
Fixes #537 |
2 participants
Description of the Changes
The "Format Code" / "Format and Indent Code" message commands and the
/format-codeslash command failed when the target message was longer than Discord's 2000-character limit: the whole snippet was wrapped in a single code block and sent as one message, which Discord rejects — so the command silently did nothing.This PR splits the formatted output into chunks that each stay under the limit:
Codeclass splits content into ≤1980-character chunks (breaking on newlines where possible) and renders each as a code block.Languageenum +Language.fromString(...)maps the slash command'sformatoption to a code-fence tag.A follow-up PR will add the language-selection dropdown.