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

Add Bruno IDE tool support and update language files #83

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Conversation

N6REJ
Copy link
Contributor

@N6REJ N6REJ commented Sep 13, 2024

. exit sandbox
. add brunoVersion = "1.28.0" to bearsampp.conf
. add https://github.com/Bearsampp/module-bruno/releases bruno 1.28.0 to "tools" in the "bruno" folder
. start sandbox
. click on "Bruno" in the tools section and verify that the ide loads.
see https://www.youtube.com/watch?v=2AlTAorlDTo for a intro into bruno
see https://docs.usebruno.com/introduction/what-is-bruno for docs

- Introduce `ToolBruno` class for managing Bruno IDE tool.
- Add Bruno tool instance in `class.tools.php`.
- Update `class.tpl.app.tools.php` to include Bruno in the tools list.
- Add Bruno symlink path in `class.symlinks.php`.
- Add Bruno language constants in `class.lang.php`.
- Update language files to include Bruno translations.
@N6REJ N6REJ added the enhancement New feature or request label Sep 13, 2024
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Error Handling
The reload method does not handle potential errors when setting the $exe property. It should check if $this->bearsamppConfRaw[self::LOCAL_CFG_EXE] exists before using it.

Code Duplication
The code for adding Bruno and Pwgen to the menu is very similar. Consider refactoring to reduce duplication.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Consistency
Correct the type value for consistency with other tool entries

Change the type of 'Bruno' from 'tools' to 'tool' to maintain consistency with other
tool entries.

core/classes/actions/class.action.quickPick.php [39]

-'Bruno'       => ['type' => 'tools'],
+'Bruno'       => ['type' => 'tool'],
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Enhancement
Add a return statement after logging that the module is not enabled

Add a return statement after logging the error when the module is not enabled to
prevent further execution.

core/classes/tools/class.tool.bruno.php [74-77]

+if (!$this->enable) {
+    Util::logInfo($this->name . ' is not enabled!');
+    return;
+}
 
-
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Possible issue
Add a check for the existence of the executable file before setting it

Consider adding a check for the existence of the executable file before setting the
$exe property to prevent potential issues if the file is missing.

core/classes/tools/class.tool.bruno.php [70-72]

 if ($this->bearsamppConfRaw !== false) {
-    $this->exe = $this->symlinkPath . '/' . $this->bearsamppConfRaw[self::LOCAL_CFG_EXE];
+    $exePath = $this->symlinkPath . '/' . $this->bearsamppConfRaw[self::LOCAL_CFG_EXE];
+    if (file_exists($exePath)) {
+        $this->exe = $exePath;
+    } else {
+        Util::logError(sprintf($bearsamppLang->getValue(Lang::ERROR_EXE_NOT_FOUND), $this->name, $exePath));
+    }
 }
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Maintainability
Group related tool items together in the menu structure

Consider grouping related tool items together for better organization and
readability of the menu structure.

core/classes/tpls/app/class.tpl.app.tools.php [62-72]

-// Bruno postman IDE
+// Development Tools
 $resultItems .= TplAestan::getItemExe(
         $bearsamppLang->getValue(Lang::BRUNO),
         $bearsamppTools->getBruno()->getExe(),
         TplAestan::GLYPH_BRUNO
     ) . PHP_EOL;
-
-// Composer
 $resultItems .= TplAestan::getItemConsoleZ(
     $bearsamppLang->getValue(Lang::COMPOSER),
     TplAestan::GLYPH_COMPOSER,
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

@jwaisner jwaisner merged commit f0b0c94 into main Sep 13, 2024
1 check passed
@jwaisner jwaisner deleted the bruno branch September 13, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants