Logger: remove not needed semicolons #247
Conversation
WalkthroughThe changes in this pull request focus on improving error handling and code formatting across several files. In Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonInterface
participant Logger
User->>PythonInterface: create()
PythonInterface->>Logger: Log creation attempt
Logger-->>PythonInterface: Log success or error
PythonInterface->>User: Return creation result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Editor/src/Scripting/PythonInterface.cpp (1)
222-222: Approve semicolon additions and suggest minor improvement.The addition of semicolons to the
osre_errorcalls is correct and necessary. These changes ensure proper syntax and prevent potential compilation errors.Consider updating the error message in
PythonInterface::destroy:- osre_error(Tag, "Error while destoy, Python interface is not created."); + osre_error(Tag, "Error while destroy, Python interface is not created.");This corrects the typo in "destoy" to "destroy".
Also applies to: 225-225, 245-245, 250-250
src/Engine/Common/Logger.h (1)
217-249: Approved: Consistent improvement across all logging macrosThe removal of semicolons from all logging macro definitions (
osre_debug,osre_info,osre_warn,osre_error, andosre_fatal) is consistent and improves the flexibility of the entire logging system. This change allows these macros to be used more robustly in various contexts.Consider using a single macro definition for all log levels to improve maintainability. For example:
#define OSRE_LOG(level, domain, msg) ::OSRE::Common::level##Print(domain, __FILE__, __LINE__, msg) // Usage: OSRE_LOG(trace, domain, msg); OSRE_LOG(debug, domain, msg); // ... and so on for other log levelsThis approach would centralize the logging logic and make it easier to maintain and modify in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/Editor/src/Scripting/PythonInterface.cpp (7 hunks)
- src/Engine/App/CameraComponent.cpp (1 hunks)
- src/Engine/Common/Logger.h (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Engine/App/CameraComponent.cpp
🧰 Additional context used
🔇 Additional comments (7)
src/Editor/src/Scripting/PythonInterface.cpp (4)
70-70: Approve semicolon addition.The addition of the semicolon after
new Project()is correct and necessary. This change improves the code's syntax and prevents potential compilation errors.
83-83: Approve blank line addition.The addition of a blank line improves code readability by separating logical blocks of code. This change adheres to good coding practices and makes the function structure clearer.
196-196: Approve semicolon additions to error logging calls.The addition of semicolons to the
osre_errorcalls is correct and necessary. These changes ensure proper syntax and prevent potential compilation errors.Also applies to: 207-207, 211-211
Line range hint
1-259: Summary of changes and recommendations.The changes in this file primarily focus on improving code correctness by adding necessary semicolons to error logging calls and initializing a global pointer. These modifications prevent potential compilation errors and adhere to good coding practices.
Key points:
- Semicolons were added to
osre_errorcalls throughout the file.- A global pointer
gActiceProjectwas initialized tonullptr.- A blank line was added in the
osre_project_loadfunction to improve readability.Recommendations:
- Fix the typo in the global pointer name from
gActiceProjecttogActiveProject.- Correct the typo in the error message in
PythonInterface::destroyfrom "destoy" to "destroy".Overall, these changes improve the code quality and maintainability.
src/Engine/Common/Logger.h (3)
159-162: LGTM: Good addition ofunregisterLogStreammethodThe new
unregisterLogStreammethod is a valuable addition to theLoggerclass. It provides symmetry with the existingregisterLogStreammethod and allows for more flexible management of log streams. The method signature is consistent with the class's coding style.
209-209: Approved: Improved macro definitionRemoving the semicolon from the
osre_tracemacro definition is a good practice. This change allows the macro to be used more flexibly in different contexts without introducing unexpected semicolons. It improves the overall robustness of the macro system.
Line range hint
1-249: Summary: Positive improvements to the logging systemThe changes made to
src/Engine/Common/Logger.hare well-implemented and improve the functionality and correctness of the logging system. Key improvements include:
- Addition of the
unregisterLogStreammethod, providing better control over log streams.- Removal of semicolons from macro definitions, improving their flexibility and robustness.
These changes enhance the usability and maintainability of the logging system. The suggestion to use a single macro for all log levels could further improve maintainability in the future.
Overall, these changes are approved and ready for merging.
| @@ -49,7 +49,7 @@ typedef struct Osre_Project { | |||
| Stage *mStage; | |||
| Project *mProject; | |||
| } Osre_Project; | |||
|
|
|||
There was a problem hiding this comment.
Fix typo in variable name and approve initialization.
The variable name gActiceProject appears to have a typo. It should be gActiveProject. However, initializing the global pointer to nullptr is a good practice.
Please apply the following change:
-Osre_Project *gActiceProject = nullptr;
+Osre_Project *gActiveProject = nullptr;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Osre_Project *gActiveProject = nullptr; |
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor