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

docs: add openAPI spec, add nix-shell #1046

Merged
merged 55 commits into from May 2, 2024
Merged

Conversation

Indyandie
Copy link
Contributor

@Indyandie Indyandie commented Feb 24, 2024

To generate pokeapi OpenAPi document run:

./manage.py spectacular --color --file openapi.yml

To start the nix-shell run:

nix-shell

Fixes #528

@Naramsim
Copy link
Member

Hi, what's nix shell? And what's the added advantage of using it?

@Indyandie
Copy link
Contributor Author

Hi, what's nix shell? And what's the added advantage of using it?

Howdy @Naramsim!

The nix-shell is an interactive shell that's created using a nix expression (in this case the default.nix). It allows you to create a project specific dev environment by running nix-shell. If done correctly it will install all the necessary programs required for the project.

The advantage is optional for most, but it let's anyone who uses nix to run nix-shell to have a local dev environment ready to go with little to no setup. This was a requirement for me because I'm running NixOS, and working with python is tricky due to the nature of the system.

@GreatNovaDragon
Copy link
Contributor

As i read this, its nix-shell is nothing that is needed, but nice to have and nothing that hurts to be included for the convenience of developers on NixOS

@Naramsim
Copy link
Member

Hi! Thanks for all the commits! When you are ready and the tests are passing you can ping me or request a review

@Indyandie
Copy link
Contributor Author

@Naramsim this is ready to go!

@Naramsim
Copy link
Member

Naramsim commented Mar 5, 2024

Ok thanks! Maybe @ryu-0729 can also review this. I'm really unsure which PR should I merge in, tbh. Both of you have made a great work

@Indyandie
Copy link
Contributor Author

Thanks @Naramsim! @ryu-0729 a review will be much appreciated 🙇

Copy link

@ryu-0729 ryu-0729 left a comment

Choose a reason for hiding this comment

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

@Indyandie @Naramsim
I have reviewed it.
The way you divided the tags and set up the schema was very helpful.
I have added a comment and whould be happy if you could check it out.

Thanks.

pokemon_v2/serializers.py Outdated Show resolved Hide resolved
@extend_schema(
description="Abilities provide passive effects for Pokémon in battle or in the overworld. Pokémon have multiple possible abilities but can have only one ability at a time. Check out [Bulbapedia](http://bulbapedia.bulbagarden.net/wiki/Ability) for greater detail.",
tags=["pokemon"],
)
class AbilityResource(PokeapiCommonViewset):
queryset = Ability.objects.all()
serializer_class = AbilityDetailSerializer
list_serializer_class = AbilitySummarySerializer

Copy link

Choose a reason for hiding this comment

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

Some retrieve APIs allow passing name as a path parameter.
I was thinking that a string would be better than an integer, but what do you think?

It can be defined by overriding the retrieve method of PokeapiCommonViewset.

    @extend_schema(
        parameters=[
            OpenApiParameter(
                name="id",
                description="This parameter can be a string or an integer.",
                location=OpenApiParameter.PATH,
                type=OpenApiTypes.STR,
            ),
        ]
    )
    def retrieve(self, request, pk=None):
        return super().retrieve(request, pk)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can treat IDs like strings

@Indyandie
Copy link
Contributor Author

@Naramsim @ryu-0729

@Indyandie
Copy link
Contributor Author

@Naramsim @ryu-0729 I put together a mockup of the pokeapi.co using this OpenAPI document and astro starlight.

@Indyandie
Copy link
Contributor Author

@Naramsim
Copy link
Member

Naramsim commented May 2, 2024

Hi! Are you still here? I got back for a while!

I think I'll merge this PR since it's too long we have it open. Honestly, I couldn't decide which one to merge and I decided to go with @Indyandie one since he seems a bit more active on GitHub than @ryu-0729 (in no way I'm criticizing your work or your profile).

@Indyandie
Copy link
Contributor Author

@Naramsim still here 🙋‍♂️

Let me know if we need anything else to merge. Probably need some instructions in the README?

@Naramsim
Copy link
Member

Naramsim commented May 2, 2024

I'll merge this otherwise another 3 months will pass :)

What kind of info do you want to include on the README? I'd put this command in the Makefile.

./manage.py spectacular --color --file openapi.yml

I was thinking that maybe we can also link the openapi.yml over at pokeapi.co

@Naramsim Naramsim merged commit cba4a0a into PokeAPI:master May 2, 2024
3 checks passed
@pokeapi-machine-user
Copy link

A PokeAPI/api-data refresh has started. In ~45 minutes the staging branch of PokeAPI/api-data will be pushed with the new generated data.

The staging branch will be deployed in our staging environment and the entire API will be ready to review.

A Pull Request (master<-staging) will be also created at PokeAPI/api-data and assigned to the PokeAPI Core team to be reviewed. If approved and merged new data will soon be available worldwide at pokeapi.co.

@pokeapi-machine-user
Copy link

The updater script has finished its job and has now opened a Pull Request towards PokeAPI/api-data with the updated data.

The Pull Request can be seen deployed in our staging environment when CircleCI deploy will be finished (check the start time of the last build).

@Indyandie Indyandie deleted the openapi-nix branch May 8, 2024 05:52
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.

OpenAPI support
6 participants