litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.62k stars 381 forks source link

new user issue with path parameter type definition #62

Closed philvarner closed 2 years ago

philvarner commented 2 years ago

I didn't realize that path parameters must have a type, so I declared one like @get(path="/{dataset_id}").

(1) I think this is a common mistake people will make with the minimal example, since it only uses uuid. Maybe adding an example of using str to the minimal example and a comment that it's required would help.

(2) I was confused at first by the error message starlite.exceptions.ImproperlyConfiguredException: path parameter must declare a type: '{parameter_name:type}' because I had added the type hint to to the method signature. This would be more useful if it actually printed the parameter name without the type and the method it was associated with. I think it should also suggest that you change it to "str" since that's what most people who make this mistake will intend.

(3) I'm usually an advocate for explicit typing, but it seems that the most common case where path parameters are just strings shouldn't require the :str addition, but I think more clearly documenting this for new users should be sufficient.

Goldziher commented 2 years ago

I didn't realize that path parameters must have a type, so I declared one like @get(path="/{dataset_id}").

(1) I think this is a common mistake people will make with the minimal example, since it only uses uuid. Maybe adding an example of using str to the minimal example and a comment that it's required would help.

(2) I was confused at first by the error message starlite.exceptions.ImproperlyConfiguredException: path parameter must declare a type: '{parameter_name:type}' because I had added the type hint to to the method signature. This would be more useful if it actually printed the parameter name without the type and the method it was associated with. I think it should also suggest that you change it to "str" since that's what most people who make this mistake will intend.

(3) I'm usually an advocate for explicit typing, but it seems that the most common case where path parameters are just strings shouldn't require the :str addition, but I think more clearly documenting this for new users should be sufficient.

Hi there @philvarner and thanks for the input. Its true that for str :str is sort of redundant - but its meant for consistency. Also - with 0.7.1 we actually are able to support all kinds of types, not only those supported by starlette - so this might actually become a thing in the future.

As for points 1 and 2: these two should defintely be resolved. Would you like to open a PR yourself? Its fine if not, it will just wait a bit until its resolved. If you do though it should be a problem:

docs are located under the /docs/ folder, and the error message can be found in starlite.routing

philvarner commented 2 years ago

Sounds good on (1). One recommendation I do have for you with that work is to allow precise custom error responses and document it well. One of the problems I've had with other frameworks is that they allow custom types in the path, but then the default error response for an invalid value is like "failed to serialize FooBar from path", which is completely useless to the user, instead of something helpful that the developer provides, e.g. "the path segment 'abcdf' is not a valid ID, it should match the pattern '[A-Z]+-[0-9]{3}'".

I'm unlikely to create a PR for that right now.

Goldziher commented 2 years ago

the error message is updated in #71 to be more verbose and instruct the user regarding what needs fixing and wher.e