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

Add schema generation hints to resolve warning #9046

Closed
wants to merge 1 commit into from

Conversation

1337joe
Copy link
Contributor

@1337joe 1337joe commented Feb 7, 2025

As a path to understanding the schema generator for #9045, I'm trying to address at least some of the warnings it currently produces.

/git/inventree/InvenTree/src/backend/InvenTree/company/serializers.py:107: Warning [CompanyList > CompanySerializer]: unable to resolve type hint for function "address". Consider using a type hint or @extend_schema_field. Defaulting to string.

String is actually the correct type in this case, but the field also needs to be nullable to be fully correct.

The two changes in this pull request accomplish the same goal: providing the nullable string type hint to the schema generator.

Adding a line to the serializer to explicitly define address produces:

address:
  type: string
  readOnly: true
  nullable: true

while simply adding the return type hint to the model produces:

address:
  type: string
  nullable: true
  description: |-
    Return the string representation for the primary address.

    This property exists for backwards compatibility
  readOnly: true

I would argue that the second option (adding type hints to the model) is the better way to go:

  1. It's less serializer-specific code that has to be maintained moving forward for simple fields like this one.
  2. The type definition is in the same place as the value that it matches for ease of maintenance.
  3. It displays the relevant documentation comment in the schema.

It's entirely possible that I'm missing something since I'm just focused on the schema, but the one (admittedly major) downside that I see to adding the hint to the model is that it doesn't match the way the existing serializers are set up.

Which route is preferable moving forward?

Copy link

netlify bot commented Feb 7, 2025

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit a7018b9
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/67a56fb25406c400087ff77e

@SchrodingersGat
Copy link
Member

Adding a return type hint to the model seems like the better solution to me. It is a "deeper" solution allowing higher level code (such as the serializers) to introspect. I suspect there are probably a number of other models which will need this attention

@1337joe
Copy link
Contributor Author

1337joe commented Feb 7, 2025

Adding a return type hint to the model seems like the better solution to me.

Perfect. I'll prefer that way when it's an option and I'll migrate from serializer to model where I'm already touching it.

Is this the best way to ask questions about coding direction?

@SchrodingersGat
Copy link
Member

Is this the best way to ask questions about coding direction?

GitHub is the best place for these discussions so we can keep everything in the same place :)

@1337joe 1337joe closed this Feb 7, 2025
@SchrodingersGat
Copy link
Member

@1337joe are you still taking charge of this one?

@1337joe
Copy link
Contributor Author

1337joe commented Feb 14, 2025

That's my intent but I started work on it right before I'm going out of town for a week so I haven't had time to dig into all I wanted to. I'll try to get a first pass pushed tonight or tomorrow with the intent of cleaning it up more when I'm back home.

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