-
Notifications
You must be signed in to change notification settings - Fork 32
Feature: URL Parameters #43
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
Conversation
Added option to provide regex-like URLs using <> brackets
|
||
@server.route("/device/<device_id>/action/<action>") | ||
@server.route("/device/emergency-power-off/<device_id>") | ||
def perform_action(request: HTTPRequest, device_id: str, action: str = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function intended to have two @server.route()
s on it?
I tried testing it as was able to get successfull responses from /device/<device_id>/action/<action>
But I just get 'connection timed out' error with no real response when I try /device/emergency-power-off/<device_id>
. I'm guessing it may have been leftover from some testing durrent development and can be removed? Or if it does actually belong something may need tweaked in order to make it functional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for testing it 👍
Yes, this is intentional, not a leftover. I did it to show that it is possible to have multiple routes for single handler, like Flask.
I also test it on ESP32-S2 TFT, including the /device/emergency-power-off/<device_id>
.
My guess is, that error you are encountering is in fact due to the action
defaulting to None
, thus entering second if
with device.turn_off()
which is raising NotImplementedError
and not returning proper HTTPResponse
.
Please check if that is the case, and if yes do you think it should be changed? (I updated the example to use print
instead of NotImplementedError
)
It is only an example but I agree that it might be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool functionality. Thank you for implementing this @michalpokusa.
One question and potential change, but beyond that this change is looking good to me. I tested most of this successfully with a Feather ESP32-S2 TFT.
@FoamyGuy Can I get an update on this PR? I already have some more functionality in mind, but I would like to finish this one first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current version of this looks good to me. Thank you @michalpokusa
One thing I think we should do in the examples for this library is migrate to using either secrets dictionary inside of secrets.py or perhaps the even new settings.toml which is the intended place for auth details moving forward I believe. I'll open a seperate issue for that as it's not really related to this change.
Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE to 10.0.0 from 9.0.2: > Merge pull request adafruit/Adafruit_CircuitPython_BLE#184 from adafruit/dhalbert-patch-1 Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 2.4.0 from 2.3.1: > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#43 from michalpokusa/feature/url_parameters Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
Added option for wildcard-like URLs in
HTTPServer.route
.It is now possible to do something like this:
and access parameters inside handler function, similarly to Django and I belive Flask.
Also updated examples to show new functionality.
There is literally no change to the API so this is 100% backwards compatible, if one does not use
<>
in route call, nothing changes for them.Additionally:
These changes should fulfill #42.