glideapps / quicktype

Generate types and converters from JSON, Schema, and GraphQL
https://app.quicktype.io
Apache License 2.0
12.5k stars 1.09k forks source link

Python docstrings are misplaced #1685

Open seiltingcr opened 3 years ago

seiltingcr commented 3 years ago

The python code generator for 3.6 and 3.7 puts the property descriptions as docstrings before the respective properties in the classes. This is incorrect because the first docstring after the class name is applied to the class, and all others disappear at runtime.

This causes the classes to be documented with the descriptions of their first properties; the properties to not have their descriptions; and the top-level object's description not to be generated at all.

Code to reproduce:

{
  "type": "object",
  "description": "Schema description",
  "properties": {
      "some property": {
          "type": "string",
          "description": "Description of the property"
      }
  }
}

This produces the following class for python 3.6 and 3.7:

class SchemaName:
    """Description of the property"""
    some_property: Optional[str] = None

What I would have expected:

class SchemaName:
    """Schema description"""
    # Description of the property
    some_property: Optional[str] = None

# alternatively:
class SchemaName:
    """Schema description

    Properties:
        some_property: Description of the property
    """"
    some_property: Optional[str] = None
kopecn commented 2 years ago

I upvote! :) a fix would make me very happy! :)

kopecn commented 1 year ago

https://docs.python.org/3.10/tutorial/controlflow.html#documentation-strings https://peps.python.org/pep-0287/

:)

seiltingcr commented 1 year ago

I took a look at the python renderer to see if this is a super easy fix, and it looks like the base class it uses is not quite flexible enough to allow this: It calls the same method (emitDescription) for both class descriptions and attribute descriptions, with no more data about what type of thing the description is for. It would be nice if someone who knows the codebase a little bit better could give a quick yes/no on the feasibility of this.

seiltingcr commented 1 year ago

There is also the fact that it produces quite antiquated-looking code. Even in this tiny example, it uses Optional[str], for instance, which I would not accept in a pull request in 2023. I am a bit wary of wasting time on a project that looks like 2 people have used it since March 2021.

kopecn commented 10 months ago

@seiltingcr I think this may have been fixed 2 months ago in a recent PR. --> https://github.com/glideapps/quicktype/pull/2443

I will be checking shortly.