encode / starlette

The little ASGI framework that shines. 🌟

https://www.starlette.io/

BSD 3-Clause "New" or "Revised" License

10.31k stars 948 forks source link

Add tests to `test_responses` #2656

Closed Orenoid closed 3 months ago

Orenoid commented 4 months ago

Summary

Add tests for some of the uncovered branches in starlette.responses, related to this issue

Checklist

[x] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
[x] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
[x] I've updated the documentation accordingly. (It seems no documentation needs update)

Orenoid commented 3 months ago

@Kludex Thanks for the approval. I noticed that you updated some logic and variable names. Do we have relevant coding standards? If so, I can follow them while working on the rest of tests in the future. And one more thing, would you prefer me to add the rest of the tests in separate PRs, with one PR per module, or should I add them with as few PRs as possible?

Kludex commented 3 months ago

Do we have relevant coding standards?

We do have a page in our docs, but for the changes I did in this PR, no. I just updated the code to use tmp_path instead of tmpdir. You didn't do anything wrong.

And one more thing, would you prefer me to add the rest of the tests in separate PRs, with one PR per module, or should I add them with as few PRs as possible?

The way you did here is perfect! A couple of tests per PR is good for me.