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

feat(preview): added service upload #979

Merged
merged 4 commits into from
Aug 18, 2023
Merged

feat(preview): added service upload #979

merged 4 commits into from
Aug 18, 2023

Conversation

StanGirard
Copy link
Collaborator

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Checklist before requesting a review

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented hard-to-understand areas
  • I have ideally added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Screenshots (if appropriate):

@vercel
Copy link

vercel bot commented Aug 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 18, 2023 0:53am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 18, 2023 0:53am

@StanGirard StanGirard temporarily deployed to preview August 18, 2023 12:49 — with GitHub Actions Inactive
@StanGirard StanGirard merged commit ce6b45e into main Aug 18, 2023
3 of 6 checks passed
@github-actions
Copy link
Contributor

Risk Level 2 - /home/runner/work/quivr/quivr/backend/core/main.py

The code seems to be well-structured and follows the SOLID principles. However, there are a few areas that could be improved for better readability and maintainability:

  1. Environment Variables: The SENTRY_DSN environment variable is being accessed directly using os.getenv(). It would be better to have a separate configuration file or class to manage all environment variables. This would make the code more maintainable and less prone to errors.

  2. Error Handling: The handle_request_validation_error function could be simplified for better readability. Instead of replacing newline and multiple spaces in the exception string, consider using a formatted string or a template string.

Here's an example of how you could refactor the handle_request_validation_error function:

@app.exception_handler(RequestValidationError)
async def validation_exception_handler(request: Request, exc: RequestValidationError):
    exc_str = str(exc).replace('\
', ' ').replace('   ', ' ')
    logger.error(f'Request: {request}, Error: {exc_str}')
    content = {
        'status_code': status.HTTP_422_UNPROCESSABLE_ENTITY,
        'message': exc_str,
        'data': None,
    }
    return JSONResponse(content=content, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY)
  1. Startup Event: The startup_event function is downloading pandoc if it doesn't exist. This could potentially slow down the startup time of the application. Consider moving this to a separate script that prepares the environment before the application starts.

🌍💼 (Environment Variables), 🚫📝 (Error Handling), 🚀🐌 (Slow Startup)


Powered by Code Review GPT

StanGirard added a commit that referenced this pull request Sep 12, 2023
* feat(docker): improved size image

* feat(preview): added upload service

* ci(aws): using matrix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant