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

fix: making import of ninja optional #42

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

maticardenas
Copy link
Owner

As reported by @StopMotionCuber, after we introduced support for Django-Ninja's test client within the module clients.py, projects which doesn't have ninja installed began failing when attempting to import the former client.

This PR aims to fix this issue while defining an optional import and validation of the ninja library.

@maticardenas maticardenas self-assigned this Jun 15, 2024
@maticardenas maticardenas linked an issue Jun 15, 2024 that may be closed by this pull request
Copy link

@StopMotionCuber StopMotionCuber left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at my comment and working on it.

See my review comment, I think in the current form the fix does not work yet. Would be nice if you could take another look on it

openapi_tester/clients.py Outdated Show resolved Hide resolved
openapi_tester/clients.py Outdated Show resolved Hide resolved
@maticardenas maticardenas force-pushed the 41-import-error-for-projects-without-ninja branch 2 times, most recently from c7b45e6 to 48d708b Compare June 17, 2024 12:33
@StopMotionCuber
Copy link

My test integration is still failing, but I guess for unrelated reasons. I'm using validate_response at some point and it seems that with 1.4.0 the expected request format has changed. Will do some deeper analysis tomorrow. This MR looks good

@StopMotionCuber
Copy link

With the fix of changing

schema_tester.validate_response(response)

to

schema_tester.validate_response(DRFResponseHandler(response))

my pipeline works again with your proposed patch. I mean it makes sense that it starts breaking if you change the API.

Here the question would be whether it makes sense to have a breaking API change or whether it would be a better idea to go with the previous code of

response_handler = ResponseHandlerFactory.create(response)

and integrate the detection "which type of response is it? DRF or Django Ninja?" there.

Anyways, I consider this comment to be out of scope for this PR and can confirm that the patch works.

@maticardenas maticardenas force-pushed the 41-import-error-for-projects-without-ninja branch from 48d708b to 0bbe5ac Compare June 18, 2024 07:31
@maticardenas
Copy link
Owner Author

@StopMotionCuber indeed validate_response's signature changed in version 1.4.0 in favor of using now the response handler (aiming for simplicity there). I agree the usage of the factory is the ideal in this case.

I will then merge this PR and trigger a release so that the issue with the absence of ninja is solved. Thanks for the help!

@maticardenas maticardenas force-pushed the 41-import-error-for-projects-without-ninja branch from 0bbe5ac to 3ff799c Compare June 18, 2024 07:41
@maticardenas maticardenas merged commit ebccfb1 into master Jun 18, 2024
14 checks passed
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.

Import error for projects without ninja
2 participants