Fix Javascript evaluation for $run and remove the global error handler. #885
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors the code execution pipeline by restructuring the run command and dispatch services into typed, doc-driven components, replaces the global error handler with structured exceptions, corrects JavaScript/TypeScript evaluation and output filtering (including Nim error suppression), and adds appropriate headers and reaction handling. Sequence Diagram for Successful '$run' Command ExecutionsequenceDiagram
actor User
participant DiscordClient as Discord Client
participant RunCog as Run Cog
participant SelectedService as Execution Service (Godbolt/Wandbox)
participant ExternalAPI as External API (Godbolt/Wandbox)
User->>DiscordClient: !run [lang] ```code```
DiscordClient->>RunCog: run(ctx, code)
RunCog->>RunCog: _extract_code_from_message(ctx, code)
opt code found
RunCog->>RunCog: _parse_code_block(extracted_code)
RunCog->>RunCog: _determine_service(language)
end
alt Service determined and code valid
RunCog->>DiscordClient: add_reaction(LOADING_REACTION)
RunCog->>SelectedService: run(language, source_code)
activate SelectedService
SelectedService->>SelectedService: _execute(compiler, source_code, options)
SelectedService->>ExternalAPI: POST /api/... (code, compiler, options)
activate ExternalAPI
ExternalAPI-->>SelectedService: HTTP Response (output)
deactivate ExternalAPI
SelectedService-->>RunCog: output_string
deactivate SelectedService
RunCog->>RunCog: _create_result_embed(output_string, language, service)
RunCog->>RunCog: _create_close_button_view()
RunCog->>DiscordClient: send(embed, view)
RunCog->>DiscordClient: clear_reaction(LOADING_REACTION)
else Service not determined or code invalid
%% This path leads to specific error handling (see Error Handling diagram)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @HikariNee - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tux/cogs/utility/run.py
Outdated
Comment on lines
97
to
98
| if ( | ||
| temp["compiler_error"] != "" and self.compiler_map.get("nim") != lang |
There was a problem hiding this comment.
issue (bug_risk): Incorrect Nim language check in compiler error condition
self.compiler_map.get("nim") returns a version string, not the language key. Compare lang directly to "nim" instead (e.g., lang != "nim").
There was a problem hiding this comment.
lang is always the compiler name, the check will fail.
There was a problem hiding this comment.
You’re absolutely right—since lang already holds the compiler key ("nim"), comparing against self.compiler_map.get("nim") (which is a version string) will never be false and Nim errors will still slip through. Let’s just compare lang directly to "nim". For example:
- if (
- temp["compiler_error"] != "" and self.compiler_map.get("nim") != lang
- ):
+ # treat Nim’s debug messages as non-errors
+ if temp["compiler_error"] != "" and lang != "nim":
return temp["compiler_error"]Or even more succinctly:
if temp["compiler_error"] and lang != "nim":
return temp["compiler_error"]That should properly skip Nim’s debug output from being treated as a “compiler_error” while still catching real errors for all other languages. Don’t forget to add (or update) a test case for "nim" so we can be sure we’re filtering its debug noise but still surfacing actual compilation failures.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @HikariNee - I've reviewed your changes - here's some feedback:
- Normalize the extracted language (e.g.
.lower()) before looking up inGODBOLT_COMPILERS/WANDBOX_COMPILERSto avoid case-sensitivity mismatches. - The generic type parameter on
discord.ui.Button[discord.ui.View]in_create_close_button_viewis not needed and could be removed to avoid potential type errors. - You could merge the two compiler maps into a single lookup or shared registry and drive
_determine_servicefrom that to eliminate duplication and keep supported languages in sync.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| @staticmethod | ||
| def __remove_backticks(st: str) -> str: | ||
| def _determine_service(self, language: str) -> str | None: |
There was a problem hiding this comment.
suggestion: Normalize language string before mapping to services
Normalize the language string (e.g., language = language.lower().strip()) before matching it to WANDBOX_COMPILERS and GODBOLT_COMPILERS to handle variations in user input.
| help_text = ( | ||
| "The following languages are currently supported by the `run` command:\n" | ||
| f"```{languages_text}\n```\n\n" | ||
| "Please use triple backticks and provide syntax highlighting like below:\n" | ||
| '```\n`\u200b``python\nprint("Hello, World!")\n`\u200b``\n```\n' | ||
| ) |
There was a problem hiding this comment.
suggestion: Fix code block quoting in supported languages help
Use \n{languages_text}\n to ensure the list is displayed as code content, not as a language specifier.
Suggested change
| help_text = ( | |
| "The following languages are currently supported by the `run` command:\n" | |
| f"```{languages_text}\n```\n\n" | |
| "Please use triple backticks and provide syntax highlighting like below:\n" | |
| '```\n`\u200b``python\nprint("Hello, World!")\n`\u200b``\n```\n' | |
| ) | |
| help_text = ( | |
| "The following languages are currently supported by the `run` command:\n" | |
| f"```\n{languages_text}\n```\n\n" | |
| "Please use triple backticks and provide syntax highlighting like below:\n" | |
| '```\n`\u200b``python\nprint("Hello, World!")\n`\u200b``\n```\n' | |
| ) |
Comment on lines
+280
to
+278
| if program_output := result.get("program_output"): | ||
| output_parts.append(str(program_output)) | ||
|
|
||
| return " ".join(output_parts).strip() if output_parts else None |
There was a problem hiding this comment.
suggestion: Preserve newlines when joining Wandbox output parts
Using spaces to join output_parts removes line breaks from multiline outputs. Use '\n'.join(output_parts) to preserve the original formatting.
Suggested change
| if program_output := result.get("program_output"): | |
| output_parts.append(str(program_output)) | |
| return " ".join(output_parts).strip() if output_parts else None | |
| if program_output := result.get("program_output"): | |
| output_parts.append(str(program_output)) | |
| return "\n".join(output_parts).strip() if output_parts else None |
| return cleaned_code.split("\n").pop(0), "\n".join(cleaned_code.splitlines()[1:]) | ||
|
|
||
| async def send_embedded_reply(self, ctx: commands.Context[Tux], output: str, lang: str, is_wandbox: bool) -> None: | ||
| # sourcery skip: assign-if-exp, reintroduce-else |
There was a problem hiding this comment.
issue (code-quality): Unused skip comment (unused-skip-comment)
… issues with Javascript, Typescript and Pascal.
Introduce new exceptions for code execution errors, including MissingCodeError, InvalidCodeFormatError, UnsupportedLanguageError, and CompilationError. These exceptions provide specific feedback to users and are integrated into the error handling configuration to log and optionally send certain errors to Sentry for monitoring. Enhancing error handling for code execution improves user experience by providing clear feedback on what went wrong and allows for better monitoring of issues related to code execution, aiding in debugging and improving system reliability.
… and documentation Add comprehensive docstrings to functions and classes for better understanding and maintainability. Introduce custom exceptions for specific error scenarios like missing code, invalid format, unsupported language, and compilation errors. Refactor code to improve readability and maintainability by using helper functions and constants. Enhance user feedback with detailed error messages and usage instructions. These changes improve the robustness and user experience of the code execution cog by providing clear error messages and documentation, making it easier for users to understand how to use the feature and for developers to maintain and extend the code.
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_2f12ad1b-2b96-4d24-9cef-515670c373cc). |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the evaluation of Javscript and typescript (and Pascal), removes the local error handler and does some visual changes.
Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
[Y] I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
By invoking it in the Tux dev server.
Screenshots (if applicable)
NA
Additional Information
NA
Summary by Sourcery
Restructure and improve the
runcog by formalizing code dispatch services, centralizing constants, and replacing monolithic parsing logic with helpers. Introduce custom exceptions for clear error reporting, fix JavaScript/TypeScript execution, and enhance embeds with a close button while removing the old global error handler.New Features:
runcommandBug Fixes:
Enhancements:
CodeDispatchbase class with separateGodboltServiceandWandboxServiceimplementations_remove_ansi,_remove_backticks) as module-level constantsDocumentation: