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

Incorrect default value for queries with @include directive #272

Closed
salmannotkhan opened this issue Jan 31, 2024 · 6 comments · Fixed by #292
Closed

Incorrect default value for queries with @include directive #272

salmannotkhan opened this issue Jan 31, 2024 · 6 comments · Fixed by #292

Comments

@salmannotkhan
Copy link
Contributor

First of all, I loved the project. It has everything that anyone can expect.

Recently I ran into an issue, When the field name is in camelCase and uses @include directive the generated pydantic code doesn't generate proper default value (None)

I have used https://graphqlzero.almansi.me/api schema for testing purpose

queries.graphql:

query foo{
  postData: post (id: 1) @include(if: true)  {
    id
  }
}

generated pydantic code:

from typing import Optional

from pydantic import Field

from .base_model import BaseModel


class Foo(BaseModel):
    post_data: Optional["FooPostData"] = Field(alias="postData")


class FooPostData(BaseModel):
    id: Optional[str]

post_data should be Field(None, alias="postData") because it's optional.

@salmannotkhan
Copy link
Contributor Author

salmannotkhan commented Jan 31, 2024

Issue seems to be here: https://github.com/mirumee/ariadne-codegen/blob/main/ariadne_codegen/client_generators/result_types.py#L391-L418

We are not assigning positional arg for default or default keyword arg. I have fixed it by adding default kwarg in there. Should I create PR for the same?

@salmannotkhan
Copy link
Contributor Author

I think we should also fix the generate_pydantic_field method. It currently only takes keywords arguments. It should accept positional arguments also.

What do you guys think?

@salmannotkhan
Copy link
Contributor Author

@rafalp Can you take a look at this issue? I have already fixed it in local repo, I can create PR for the same.

@ori-lb
Copy link

ori-lb commented Apr 16, 2024

Is this issue fixed on master? encountering the same problem.
@salmannotkhan did you raise a pr eventually?

@salmannotkhan
Copy link
Contributor Author

Waiting for @rafalp's permission.

@bombsimon
Copy link
Contributor

I don't think you need permission to create a PR. I would suggest just creating it so it can be reviewed when maintainers have the time and while waiting hopefully others will see it and be able to test it or spot things that might require further changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants