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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute all routes from FastAPI app once #24

Closed

Conversation

wo0dyn
Copy link
Contributor

@wo0dyn wo0dyn commented Mar 15, 2023

Rationale

There were 2 reasons for this PR:

  1. It felt a bit too intrusive to keep a reference to the app in the HyperModel
  2. We can compute all the routes at once instead of iterating on them in HALFor.__build_hypermedia__().

Note
Feel completely free to reject on of those commit; even the last (main) one. 馃槉

ChangeLog

  • Refactoring
    • Remove superfluous object
    • Remove Pydantic (already dependency in FastAPI)
    • Remove unused dev dep: requests
    • Remove unused Hypermodel private method
  • Lazy compute all routes from FastAPI app once

I let you check each commit message for more details.

I've used the tool pyupgrade[^1] to perform this cleanup, using the
following command:

```console
$ find examples fastapi_hypermodel tests -name "*.py" \
    -exec pyupgrade --py37-plus {} \;
```

It could be useful to add this weapon in tox test suite for instance.

[^1]: https://github.com/asottile/pyupgrade
I would remove Pydantic as a dependency for this project as it's already
a dependency of FastAPI[^1] itself.

[^1]: https://github.com/tiangolo/fastapi/blob/master/pyproject.toml#L45
Unless I'm mistaken, these two dependencies are not used in the project.
I would remove them as well.
I didn't find any usage of this method in the project. And since it was
defined with a protected scope (by convention), I don't think it should
be used somewhere else.
@wo0dyn wo0dyn changed the title refactor/remove bound app in hypermodel Remove bound app in HyperModel Mar 15, 2023
@wo0dyn wo0dyn force-pushed the refactor/remove-bound-app-in-hypermodel branch 3 times, most recently from 7f30819 to c46ce44 Compare March 16, 2023 01:16
Instead of keeping a reference to the FastAPI app, I think it's better
to keep its routes. This way, on `HALFor()`, it's more convenient to
retrieve a given route by its endpoint (aka route.name) rather than
iterate on app.routes at every call.
@wo0dyn wo0dyn force-pushed the refactor/remove-bound-app-in-hypermodel branch from c46ce44 to 0f58218 Compare March 16, 2023 01:21
Copy link
Contributor Author

@wo0dyn wo0dyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

@@ -64,7 +64,7 @@ class UrlFor(UrlType):
At this point, our custom type will behave as a normal Pydantic type, but won't do any hypermedia substitutions.
For this, we must add our "magic" `__build_hypermedia__` method.

```python hl_lines="32-38"
```python hl_lines="32-37"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope the range is still correct. 馃槰

fastapi_hypermodel/hypermodel.py Show resolved Hide resolved
fastapi_hypermodel/hypermodel.py Show resolved Hide resolved
@wo0dyn wo0dyn marked this pull request as ready for review March 16, 2023 01:29
@wo0dyn wo0dyn changed the title Remove bound app in HyperModel Compute all routes from FastAPI app once Mar 17, 2023
jtc42 and others added 2 commits March 19, 2023 17:54
Co-authored-by: Nicolas Dubois <nicolas.c.dubois@gmail.com>
@wo0dyn wo0dyn closed this Apr 14, 2023
@wo0dyn wo0dyn deleted the refactor/remove-bound-app-in-hypermodel branch April 14, 2023 18:41
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

2 participants