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

Maint/#217 #230

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Maint/#217 #230

merged 2 commits into from
Dec 15, 2023

Conversation

PGijsbers
Copy link
Collaborator

This specifically only annotates those endpoints which has a Path or Query in their signature.
However, it may be useful to annotate all endpoint parameters this way, to explicitly state whether they are Query, Path, or Body parameters and provide them with additional metadata (description, example).

@@ -67,7 +67,7 @@ def indexed_fields(self) -> set[str]:
def create(self, url_prefix: str) -> APIRouter:
router = APIRouter()
read_class = resource_read(self.resource_class) # type: ignore
indexed_fields = Literal[tuple(self.indexed_fields)] # type: ignore
indexed_fields: TypeAlias = Literal[tuple(self.indexed_fields)] # type: ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allows us to remove a # type: ignore for search_fields below. docs

@josvandervelde josvandervelde self-requested a review December 12, 2023 14:27
Copy link
Contributor

@josvandervelde josvandervelde left a comment

Choose a reason for hiding this comment

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

Thanks Pieter, looks great!

Out of curiosity: we normally don't fork the project. I personally find it easier to just switch to a different brand instead of checking out a fork, if I want to test out the PR. You probably created a fork because you didn't have the correct access rights yet, I guess? Or do you generally like it better to create a fork?

@josvandervelde
Copy link
Contributor

Anyway, feel free to merge it!

@PGijsbers
Copy link
Collaborator Author

Indeed, I forked because I did not have write access to the repository at the time. As long as people are mindful of their branches (i.e., enable auto-delete after merge, curate their own stale branches), then I also prefer working with branches in a repository over branches in forks.

@PGijsbers PGijsbers merged commit 5a25788 into aiondemand:develop Dec 15, 2023
1 check passed
@PGijsbers PGijsbers deleted the maint/#217 branch December 15, 2023 08:37
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