Closed N6REJ closed 1 month ago
⏱️ 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. |
Category | Suggestion | 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 othertool entries.** [core/classes/actions/class.action.quickPick.php [39]](https://github.com/Bearsampp/sandbox/pull/83/files#diff-d1f6e26b10d84a231fa30e52206aab0eb80b30eb4a24c22c2fabefb20384ba61R39-R39) ```diff -'Bruno' => ['type' => 'tools'], +'Bruno' => ['type' => 'tool'], ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 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 toprevent further execution.** [core/classes/tools/class.tool.bruno.php [74-77]](https://github.com/Bearsampp/sandbox/pull/83/files#diff-1feb38ee654f5876cbacb4db06298affc36a9f08bf8b28de7904a144db67a315R74-R77) ```diff +if (!$this->enable) { + Util::logInfo($this->name . ' is not enabled!'); + return; +} - ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 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]](https://github.com/Bearsampp/sandbox/pull/83/files#diff-1feb38ee654f5876cbacb4db06298affc36a9f08bf8b28de7904a144db67a315R70-R72)
```diff
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]: 7Why: | 7 |
Maintainability |
Group related tool items together in the menu structure___ **Consider grouping related tool items together for better organization andreadability of the menu structure.** [core/classes/tpls/app/class.tpl.app.tools.php [62-72]](https://github.com/Bearsampp/sandbox/pull/83/files#diff-c652ad18c42fcdf4d5763baa4439e6c4578f2cffbcc8d367f29961b7792775fcR62-R72) ```diff -// 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]: 7Why: | 7 |
. 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