Skip to content

Conversation

justin-p
Copy link
Contributor

@justin-p justin-p commented Oct 2, 2024

This PR changes the following

  • Refactor/restructure folders/code and add more test coverage .

    • Moves CRUD and Models to folders and creates a file per section. This should make it easier to apply future updates to the backend from the template to your own code base.
      • Each of their classes/functions being exported under app.models or app.crud via the __init__.py to prevent circular imports in combination with TYPE_CHECKING.
        image
      • Added more CRUD operations.
      • Every route now uses the abstracted CRUDs instead of sqlmodel directly.
    • Adds descriptive comments to all functions for an easier end user experience in for e.x. vscode
      image
    • Moves python scripts (initial_data.py, backend_pre_start.py, test_pre_start.py) in the backend to a scripts folder as additional clean up.
    • Updates any paths/imports to the new locations.
    • Updates the included documentation to reflect these changes.
  • Security improvements

    • Changed the API responses from recover_password to avoid it being able to be abused for user enumeration.
  • Misc changes

    • Added more tests to get coverage to 96%.
    • Set the docs and redocs url to live under settings.API_V1_STR
    • Ensure tests that reset the user password are performed last with @mark.order("last") to avoid issues with other tests.
    • Sets the default SECRET_KEY to a dummy value of 32 so tests pass if left unchanged.
    • Makes the included bash scripts executable by default.
    • Adds a .coveragerc file to exclude models, tests files themselves, etc, from being include in the test coverage.
    • Adds a pytest.ini for more control over testing.
    • Added pyright ignore's to stop vscode from complaining.

@justin-p justin-p marked this pull request as draft October 2, 2024 21:29
@justin-p justin-p marked this pull request as ready for review October 2, 2024 21:43
@justin-p justin-p changed the title Refactor the API ♻️ Refactor the API Oct 2, 2024
@tiangolo
Copy link
Member

Thanks for the interest! I see this has lots of changes packed in a single PR. It would be better to separate each atomic suggestion in an independent PR that can be individually reviewed.

I see a lot of the changes are new CRUD functions, an older version of this project had lots of them, and I actually did put a lot of effort into refactoring it to remove all the extra complexity. 😅

Also, the docstrings depend on the specific format, I would prefer to use what I use for FastAPI, and right now I would rather not have docstirngs in these functions yet.

Also, separating models into files can lead to complex logic to handle imports, references, cyclic imports, etc. So I prefer to have the simplest structure possible and allow people to refactor and evolve as needed, you can read more about that in the SQLModel docs.

For now I'll pass on this one, but thanks for the effort! ☕

@tiangolo tiangolo closed this Oct 12, 2024
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.

2 participants