adafruit / Adafruit_CircuitPython_HTTPServer

Simple HTTP Server for CircuitPython
MIT License
46 stars 30 forks source link

Case insensitive HTTPHeaders, HTTPResponse context manager and some fixes #29

Closed michalpokusa closed 1 year ago

michalpokusa commented 1 year ago

Added separate class for HTTP headers, that allows accessing headers using case-insensitive names.

Also, changed some docstrings the were unclear and one that was misleading as it was inconsistent with the code.

Changed the order of Docs References, so that the most "important" ones are on the top, with enums and dataclass-like classes below.

Updated library version to 1.1.0 in sphinx configuration file.

Fixes #26 Fixes #28

anecdata commented 1 year ago

@michalpokusa Any thoughts on this 404 issue? https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer/issues/28#issuecomment-1362933127 I don't see anything obvious in this PR that would have affected it. Maybe it came in with the refactor and just wasn't noticed until now. Worth a separate issue?

michalpokusa commented 1 year ago

@michalpokusa Any thoughts on this 404 issue? #28 (comment) I don't see anything obvious in this PR that would have affected it. Maybe it came in with the refactor and just wasn't noticed until now. Worth a separate issue?

Working on it, also I see that other PR was merged and there are merge conflicts, so I will fix both before proceeding

michalpokusa commented 1 year ago

After merging recently updated main branch i noticed some inconsistencies wit hthe HTTPResponse API, mainy the fact that some function would accept conn and some not. Also in chunked response sending headers and empty chunk to finish transfer should be implicit.

I decided to modify the HTTPResposne API to be unified by using context managers, by doings so i was able to reduce abstraction and number of lines of actual code, the PR may not show this as I added/extended some docstrings.

Also fixed the 404 problem in #28.

Using context managers all of the examples below are allowed. image

paul-1 commented 1 year ago

That for sure looks a lot cleaner. Thanks for doing that

michalpokusa commented 1 year ago

In the meantime I updated examples to new "context manager way" of serving response. Did some testing on my own and I believe that it works as it should. Also I added raising error if someone tries to use .send() or .send_file() multiple times per response.

michalpokusa commented 1 year ago

@anecdata Could be please take a look at the current state of this PR. I believe there is everything I wanted to add and that it is ready to be merged.

In the c758e51 commit I also "included" the #30.

dhalbert commented 1 year ago

This should be 2.0.0 if there are incompatible API or use changes. A major version change means a code rewrite is needed.

michalpokusa commented 1 year ago

This should be 2.0.0 if there are incompatible API or use changes. A major version change means a code rewrite is needed.

If I understand correctly, the code stays the same, only tag changes, so no further edits are needed on my part to make it 2.0.0

bill88t commented 1 year ago

Closing #30 then. Thanks for taking care of it!

dhalbert commented 1 year ago

If I understand correctly, the code stays the same, only tag changes, so no further edits are needed on my part to make it 2.0.0

Right, when I or whoever makes a release, it will be 2.0.0. There are incompatible API or use changes, right?

michalpokusa commented 1 year ago

There are incompatible API or use changes, right?

Yes, instead of returning HTTPResponse instance in route handlers, the function should call a method on HTTPResponse, so all handlers need to be slightly modified, but still.

dhalbert commented 1 year ago

Great - I will make a release now.

I only found one use of this library in the Learn guides: https://github.com/adafruit/Adafruit_Learning_System_Guides/blob/main/PicoW_CircuitPython_HTTP_Server/code.py Could someone take a look at that and see if there are any changes to be made? Thanks.

michalpokusa commented 1 year ago

Great - I will make a release now.

I only found one use of this library in the Learn guides: https://github.com/adafruit/Adafruit_Learning_System_Guides/blob/main/PicoW_CircuitPython_HTTP_Server/code.py Could someone take a look at that and see if there are any changes to be made? Thanks.

Thanks for merging.

Yes, there will be a small change in base and buttonpress functions. I could make the required changes, but I will be unable to test it on intended hardware as I do not own a Raspberry Pico W.

anecdata commented 1 year ago

Maybe if Liz still has it set up, it would be easy for her to test. I have a Pico W and could test the routes, but I don't have the peripherals, so I'd stub out the I/O (possibly introducing or masking an issue).

Any network code written for raspberrypi should run on espressif (but not necessarily vice versa since there are some things not implemented / not implementable).

dhalbert commented 1 year ago

If you either of you could make a draft PR, I'll test it by simplifying or setting up the base hardware. Thanks.

anecdata commented 1 year ago

https://github.com/adafruit/Adafruit_Learning_System_Guides/pull/2375