Helper to convert api_endpoint into DecodedURL #223
Conversation
Codecov Report
@@ Coverage Diff @@
## master #223 +/- ##
==========================================
+ Coverage 87.15% 87.25% +0.09%
==========================================
Files 24 25 +1
Lines 2685 2706 +21
Branches 348 350 +2
==========================================
+ Hits 2340 2361 +21
Misses 256 256
Partials 89 89
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| _ENDPOINT_CONVERTERS = { | ||
| u"tcp": _tcp_to_http_api_root, | ||
| u"ssl": _ssl_to_https_api_root, | ||
| } |
There was a problem hiding this comment.
Perhaps error handling could be improved here? As in, what happens when endpoint_description is not really an endpoint?
I just learned that magic-folder migrate --listen-endpoint 6001 --node-directory ../path/to/node accepts that non-endpoint argument without complaining. Thus magic folder configuration will have just 6001 for api_endpoint, and passing that on will result in an error here.
Of course migrate too probably could some error checking...
There was a problem hiding this comment.
Yea, it would be good to reject that early. create_global_configuration should probably reject it and then the migrate command could handle that exception and report something meaningful to the cli user.
By the time it makes it into the database, it should really be valid and no one else in the system should have to worry about validation.
| from ._endpoint_parser import ( | ||
| endpoint_description_to_http_api_root, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Would it make sense to add an additional helper in GlobalConfigDatabase, like this:
def http_api_root(self):
"""
Base URL for HTTP API, derived from endpoint describtion.
"""
return endpoint_description_to_http_api_root(self.api_endpoint)Perhaps consumers of the code in config.py can avoid some duplication that way?
There was a problem hiding this comment.
It might make sense, I'm not sure. It might also come down to taste. I almost made it a method but decided against it based on the idea that GlobalConfigDatabase's job is to allow state to be stored and retrieved, not to provide all of the different convenient interfaces to that state. Adding one accessor is hardly a terrible burden but maybe it's a step in the direction we don't want to go, so I'm inclined to leave it off for now and then add it later if it turns out it would be a significant benefit (we could also consider adding the above except as a free function instead of a method which might be 99% of the convenience without expanding GlobalConfigDatabase's interface.
Fixes #222
There didn't seem to be any particular reason to tie this tightly to GlobalConfigDatabase.api_endpoint so I didn't. It's just a helper you can use with that value.