-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix code style and linting errors #223
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving code style and fixing linting errors across multiple files in the project. The changes primarily involve formatting adjustments, variable renaming for consistency, and minor syntax improvements to adhere to the project's style guidelines. Key modifications include:
These changes aim to enhance code readability, maintainability, and consistency throughout the codebase. File-Level Changes
Tips
|
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 @untari - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential hard-coded secret in authorization header. (link)
- Potential hard-coded URL in QR code generation. (link)
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 2 blocking issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
try { | ||
this.isExporting = true | ||
const url = config.api.base + 'export-talk?export_type=' + this.selectedExporter.id | ||
const authHeader = api._config.token ? `Bearer ${api._config.token}` : (api._config.clientId ? `Client ${api._config.clientId}` : null) |
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.
🚨 issue (security): Potential hard-coded secret in authorization header.
Using api._config.token
and api._config.clientId
directly in the code can expose sensitive information. Consider fetching these values from environment variables or a secure vault.
if (!['ics', 'xml', 'myics', 'myxml'].includes(option.id)) { | ||
return | ||
} | ||
const url = config.api.base + 'export-talk?export_type=' + option.id |
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.
🚨 issue (security): Potential hard-coded URL in QR code generation.
Using config.api.base
directly in the code can expose sensitive information. Consider fetching this value from environment variables or a secure vault.
Please revert all the changes that adds whitespace between function name and |
Hi @hongquan, I double-checked the upstream code, and it also uses a space between the function name and the parameters (). According to the ESLint documentation, I could adjust the rules to remove the space, but I want to clarify this with both the upstream code and the ESLint documentation. Currently, if I remove the space, it won't pass the ESLint checks. |
fix tests it's to resolve flake8 |
@untari Could you help adjust ESLint configuration, removing "space-before-function-paren" rule? Later on, I plan to use these tools for linting:
|
alright will do! thanks
…On Sat, Aug 17, 2024, 11:30 AM Nguyễn Hồng Quân ***@***.***> wrote:
@untari <https://github.com/untari> Could you help adjust ESLint
configuration, removing "space-before-function-paren" rule?
Our JS code primarily targets VueJS, and we should follow VueJS style (no
space between function name and "(", no semicolon at the end of line etc.).
Later on, I plan to use these tools for linting:
- For Python code, use Ruff <https://docs.astral.sh/ruff/> in place of
flake8, isort.
- For JS, reconfigure ESLint to follow VueJS style.
—
Reply to this email directly, view it on GitHub
<#223 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALWSXNFHCPKM335KGKNTA43ZR27T7AVCNFSM6AAAAABMNUU2SSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJUGYYDMMZYGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi @mariobehling @hongquan could you review the latest changes for flake8, black, isort and lint. These tests are passed. If there is no further feedback, please merge this. So I don't have to keep fixing the new code that's not following the style guide and start to enforcing to use |
This PR contributes to resolving issues: Tests: Fix isort, flake8 code style, Postgres and Python tests
The changes ensure that the code adheres to the project's style guidelines, improving overall code quality and maintainability.
Testing:
Summary by Sourcery
Fix code style and linting errors across the project to improve code quality and maintainability.
Enhancements: