-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Clarification Changes #9141
base: development
Are you sure you want to change the base?
Clarification Changes #9141
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving code clarity and documentation across the project. It includes minor changes to code comments, function descriptions, and removes several unused script files. The changes are primarily in Python files and documentation, with a significant number of script deletions. Updated class diagram for SQLiteDateTimeTypeclassDiagram
class SQLiteDateTimeType {
+Integer impl
+datetime epoch
+process_bind_param(value, dialect)
+process_result_value(value, dialect)
}
note for SQLiteDateTimeType "Added detailed comments for methods and attributes"
Updated class diagram for string modification functionsclassDiagram
class StringModification {
+remove_line_breaks(target_string: str) str
+strip_line_breaks(target_string: str) str
+clean_up_string(target_string)
+clean_html(html, allow_link)
+strip_tags(html)
}
note for StringModification "Added detailed comments for each function"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @LincolnOlsen - I've reviewed your changes - here's some feedback:
Overall Comments:
- Thank you for the code clarifications and documentation improvements. The added comments in the SQLite datetime handling and the note for Windows users in the installation guide are particularly helpful. However, please provide an explanation for the removal of multiple script files in the
scripts/
directory. What is the rationale behind these deletions, and is their functionality being replaced elsewhere in the project? - For future pull requests, please include a more detailed description of all significant changes, especially when removing files or making structural modifications to the project. This helps reviewers understand the context and impact of the changes.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -7,6 +7,9 @@ | |||
* Postgres | |||
* OpenSSL | |||
|
|||
### A quick note for Windows users | |||
Some files within the migrations/versions directory contain illegal file names for a Windows system. The Open Event Server is not optimized for running locally on a Windows machine. Therefore, it is suggested that you use Windows Subsystem for Linux (WSL) or a virtual machine to run the project locally. The setup steps for WSL can be found here: https://learn.microsoft.com/en-us/windows/wsl/install. Once using one of these options, you should be able to complete the following linux setup steps within WSL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (documentation): Consider using markdown link format for the WSL setup instructions.
For consistency with markdown formatting, you might want to use the following format: WSL setup instructions
Some files within the migrations/versions directory contain illegal file names for a Windows system. The Open Event Server is not optimized for running locally on a Windows machine. Therefore, it is suggested that you use Windows Subsystem for Linux (WSL) or a virtual machine to run the project locally. The setup steps for WSL can be found here: https://learn.microsoft.com/en-us/windows/wsl/install. Once using one of these options, you should be able to complete the following linux setup steps within WSL. | |
Some files within the migrations/versions directory contain illegal file names for a Windows system. The Open Event Server is not optimized for running locally on a Windows machine. Therefore, it is suggested that you use Windows Subsystem for Linux (WSL) or a virtual machine to run the project locally. The [WSL setup instructions](https://learn.microsoft.com/en-us/windows/wsl/install) can be found here. Once using one of these options, you should be able to complete the following linux setup steps within WSL. |
For Azure
Added secret key
Fixes #
Added small code and documentation contributions to help with the clarity of the project.
Short description of what this resolves:
Allows other contributors to get a better understanding of the project.
Changes proposed in this pull request:
Very minor changes with files under the project, makes the code more clear either through editing the code or editing the comments.
Checklist
development
branch.Summary by Sourcery
Enhance code clarity by adding explanatory comments to functions and methods, and update installation documentation with guidance for Windows users.
Enhancements:
Documentation: