-
Notifications
You must be signed in to change notification settings - Fork 9
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
Allow custom urlpath to be specified #205
Conversation
@@ -1,6 +1,7 @@ | |||
[WOF] | |||
Network: TEST | |||
Vocabulary: TESTVocab | |||
URLPATH: TESTURL | |||
Menu_Group_Name: TEST OBSERVATIONS | |||
Service_WSDL: http://not.a.real.url/ |
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.
I know you're not changing Service_WSDL
here, but it seems like this would not be needed anymore, right? It can be constructed automatically from URLPATH
, like this (assumes WOF 1.1): URLPATH/soap/cuahsi_1_1/.wsdl
If that's true, it should be removed from here and handled automatically. But this could be done in a follow up PR if you'd like, so that this PR is not delayed.
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.
@emiliom Yea, I think that's possible to do, Service_WSDL can be auto-generated. I can work on that next. Thanks.
@@ -46,6 +46,7 @@ def config_from_file(self, file_name): | |||
|
|||
self.network = config.network.lower() | |||
self.vocabulary = config.vocabulary.lower() | |||
self.urlpath = config.urlpath.lower() |
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.
Could the lower()
enforcement cause a problem in servers / OS's that allow for case-sensitive url's (other than the domain name, which per HTTP rules can not be case sensitive)? Is there a reason to force it to lower case?
@@ -50,6 +50,7 @@ def config_from_file(self, file_name): | |||
|
|||
self.network = config.network.lower() | |||
self.vocabulary = config.vocabulary.lower() | |||
self.urlpath = config.urlpath.lower() |
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.
Could the lower()
enforcement cause a problem in servers / OS's that allow for case-sensitive url's (other than the domain name, which per HTTP rules can not be case sensitive)? Is there a reason to force it to lower case?
# http://serverip:port/networkcode/soap/cuahsi_1_0/.wsdl | ||
Service_WSDL: http://127.0.0.1:8080/odm2timeseries/soap/cuahsi_1_0/.wsdl | ||
# http://serverip:port/urlpath/soap/cuahsi_1_0/.wsdl | ||
Service_WSDL: http://127.0.0.1:8080/wofpysqlite/soap/cuahsi_1_0/.wsdl |
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.
See my other comment on test_config.cfg
about Service_WSDL
now being obsolete (and even worse, if it's inconsistent with URLPATH
).
# http://serverip:port/networkcode/soap/cuahsi_1_1/.wsdl | ||
Service_WSDL: http://127.0.0.1:8080/odm2timeseries/soap/cuahsi_1_1/.wsdl | ||
# http://serverip:port/urlpath/soap/cuahsi_1_1/.wsdl | ||
Service_WSDL: http://127.0.0.1:8080/wofpysqlite/soap/cuahsi_1_1/.wsdl |
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.
See my other comment on test_config.cfg
about Service_WSDL
now being obsolete (and even worse, if it's inconsistent with URLPATH
).
path = wof_inst.network.lower() | ||
servicesPath = '/'+wof_inst.network.lower() | ||
path = wof_inst.urlpath.lower() | ||
servicesPath = '/'+wof_inst.urlpath.lower() |
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.
See my other comment about url case sensitivity and forcing lowercase url.
servicesPath = '/'+wof_obj_1_0.network.lower() | ||
|
||
path = wof_obj_1_0.urlpath.lower() | ||
servicesPath = '/'+wof_obj_1_0.urlpath.lower() |
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.
See my other comment about url case sensitivity and forcing lowercase url.
Alrighty. My comments boil down to two:
Thanks! Awesome that you were able to find a solution to this issue! |
I forgot to add that this topic is related to #191 |
I like url's being lower case, and I was just following what the code did already, easier for search and replace. |
So do I!! But other people may choose to define a URLPATH that includes case.
Good to know (I figured). At least, if we stick to lowercase enforcement, we're sticking with current WOFpy conventions.
Ok, so that's a coding convenience. Definitely a good reason, but not necessarily a reason to override user preferences 😺 |
Overview
This PR allows for a custom urlpath so it doesn't need to match the network code.