Skip to content

feat!: Change the Response.status_code type to the HTTPStatus enum #665

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

Merged
merged 9 commits into from
Sep 18, 2022

Conversation

asfaltboy
Copy link
Contributor

@asfaltboy asfaltboy commented Aug 27, 2022

In the OpenAPI/Swagger 3.0 spec docs, in the section on responses, it's mentioned that the property name will be HTTP Response codes as per the IANA spec:

The HTTP Status Codes are used to indicate the status of the executed operation. The available status codes are defined by RFC7231 and registered status codes are listed in the IANA Status Code Registry.

It continues to add two special cases, which are currently out of scope of this PR (see below for future work).

This PR includes two main changes:

  • store int code in a HTTPResponse type, catching a ValueError as before - for both int and HTTPResponse
  • update the error message

Note: my initial aim was to add default responses (as mentioned in #124), but I thought I'd start with a smaller PR, to help me find my way around the project and help build confidence this is the right approach.
The following steps would be to:

  1. Support the special error response form: 1XX, 2XX, 3XX, 4XX, and 5XX
  2. Support default response (no error code given).
    I would probably implement this using a new wrapper type (we can call it OpenAPIResponseParameter) that is a Union of HTTPResponse with a new set of Literal types.

(p.s: I'm not sure when I will have a stretch of a few hours again, but hopefully the info above is sufficiently detailed for anyone else to continue the work)

taken from python standard library, exists from python 3.5, described
in [the documentation][1] as:

> A subclass of enum.IntEnum that defines a set of HTTP status codes,
> reason phrases and long descriptions written in English.

[1]: https://docs.python.org/3/library/http.html
the spec requires this to be a valid HTTP code, invalid integers
should not be supported
apparently it was renamed, asked to learn more here:
knope-dev/knope#254
@asfaltboy asfaltboy force-pushed the use-standard-error-codes branch from dbc7212 to 5bea94f Compare August 27, 2022 17:19
@asfaltboy asfaltboy marked this pull request as ready for review August 27, 2022 17:32
@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #665 (7f542d1) into main (e2cb382) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #665   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines         1795      1797    +2     
=========================================
+ Hits          1795      1797    +2     
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
openapi_python_client/parser/responses.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dbanty
Copy link
Collaborator

dbanty commented Aug 27, 2022

Thanks for the PR! I did some digging to find out why httpx isn't already using the builtin enum and it looks like they have their own internal enum with a bunch of helper methods on it for determining the type of code.

It's a shame they don't expose their own, since I believe that's the actual type that we're capturing, not int. 🤷

dbanty
dbanty previously approved these changes Aug 27, 2022
Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Even though this is really fixing our compatibility with the spec—it will disallow OpenAPI documents that previously generated code fine (if they have unsupported HTTP codes), so I'll mark this is a breaking change.

@dbanty dbanty added the 🥚breaking This change breaks compatibility label Aug 27, 2022
@dbanty dbanty added this to the 0.12.0 milestone Aug 27, 2022
@asfaltboy
Copy link
Contributor Author

I'm going to out for the day, later when back, I can do this:

  1. rebase over main (remove the redundant commit)
  2. use the feat! type and indicate BREAKING CHANGE in the commit, I think it should help it appear automatically as a breaking change in the release notes
  3. update the changelog , and test if knope supports the above

@dbanty
Copy link
Collaborator

dbanty commented Aug 28, 2022

Don’t worry about doing all that, I squash on merge so I can make sure the commits look right.

Thanks for the attention to detail, though!!

@asfaltboy asfaltboy changed the title Use standard error codes feat!: use standard error codes Aug 29, 2022
@asfaltboy
Copy link
Contributor Author

asfaltboy commented Aug 29, 2022

I squash on merge so I can make sure the commits look right

Nice, so I won't need to update the changelog manually in the PR, and you'd only need to add the BREAKING CHANGE: note when squashing; cool 👍

Regarding the failing tests in CI, I tried reproducing the same issue by installing a fresh 3.7 env in poetry, installing dependencies and running:

TASKIPY=true poetry run pytest --cov=openapi_python_client --cov-report=term-missing tests end_to_end_tests/test_end_to_end.py --basetemp=tests/tmp
...
======================================================================================= test session starts ==========================================================================================
platform darwin -- Python 3.7.8, pytest-6.2.5, py-1.10.0, pluggy-0.13.1
rootdir: /Users/pavelsavchenko/github/openapi-python-client, configfile: pyproject.toml
plugins: cov-2.12.1, anyio-3.3.0, mock-3.6.1
collected 372 items

tests/test___init__.py .....................................                                                                                                                                     [  9%]
...
end_to_end_tests/test_end_to_end.py ..                                                                                                                                                           [100%]

---------- coverage: platform darwin, python 3.7.8-final-0 -----------
...
TOTAL                                                                             1788      2    99%


========================================================================================= 372 passed in 17.27s ===========================================================================================

Would you know how to resolve this? Could this be coming from the cache step, where do?:

/usr/bin/tar --use-compress-program unzstd -xf /home/runner/work/_temp/e43cafd0-1f39-4c5d-9b22-95fc04289cd4/cache.tzst -P -C /home/runner/work/openapi-python-client/openapi-python-client

@dbanty dbanty changed the title feat!: use standard error codes feat!: Change the Response.status_code type to the HTTPStatus enum Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🥚breaking This change breaks compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants