-
Notifications
You must be signed in to change notification settings - Fork 4
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
Adding language support to Birdy #164
Conversation
Added language parameter to WPSClient and BirdyCLI classes to bring in line with changes to OWSLib. Only works with updated OWSLib module.
Was only used while playing with tests and learning about the code base. Forgot to remove.
Co-Authored-By: Trevor James Smith <[email protected]>
birdy/client/base.py
Outdated
""" | ||
self._converters = converters | ||
self._interactive = progress | ||
self._mode = ASYNC if progress else SYNC | ||
self._notebook = notebook.is_notebook() | ||
self._inputs = {} | ||
self._outputs = {} | ||
self._language = language |
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.
This is not necessary (the language
property takes its value in self._wps.language
)
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.
Oh, right. Didn't remove this one after adding the language property functions.
birdy/cli/base.py
Outdated
""" | ||
def __init__(self, name=None, url=None, caps_xml=None, desc_xml=None, **attrs): | ||
def __init__(self, name=None, url=None, caps_xml=None, desc_xml=None, language=None, **attrs): |
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'm not sure this is doing what you want for the cli... I think you want to add the language option in _update_commands
.
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.
hmm.. I added it mainly so it could be passed on for self.wps = WebProcessingService(...), so an instance of the BirdyCLI class could be initiated with a language, but it might be a lack of comprehension of how click.MultiCommand works and is used.
Should I remove the language parameter from the init and WebProcessingService instantiation and only add it as a command?
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'm not familiar enough with click either... I would need to check into this more.
If you want, you can focus this PR on adding language support for the client only, or find how to add it as a parameter to the cli also.
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.
Alright, well I don't think adding it to the cli is bad per say, as it still is a class that has a wps as a parameter, which can receive a language parameter. It might not be a best practice for click though...
I have time to dig a bit more into click. If it turns out to be a time sink or I get submerged by other assignments, I'll revert what I changed to BirdyCli and we'll save it for a future PR.
…to add-multiple-lang-support
Status ? |
@f-PLT I should have some time today to look at this. |
I tested my modifications of the WPSClient class with a local copy of OWSLib that includes the language parameter changes, so that aspect should be ok. For BirdyCli, I have a working prototype of the language command, but requires more testing, which I have not had time to do, having been assigned other tasks in the meantime. It's also the first time I try my hands at a Click.MultiCommand implementation, so I expect improvements/corrections can be made. In all cases, will require a release of OWSLib that includes the language parameter, and should be pretty quick once it's out. |
@cehbrecht Possible to have a minor owslib release for the language support ? |
@huard I will prepare a 0.19.2 owslib release ... |
@f-PLT take a look at 993aff5 You can test with https://finch.crim.ca/wps |
Perfect, will get on it tomorrow morning. |
@f-PLT tests should pass with the changes in this branch: https://github.com/bird-house/birdy/tree/pr-164-fix-tests |
test_help() and test_show_languages() require an online wps (Emu, in this case)
Integrated David's changes. CLI tests however now require to be online tests (with Emu). I also tested Birdy manually and everything seems to work. I do, however, get the three following test failures for the full test suites.
I'm using a docker deployment of Emu, as per the documentation. I also get those failures if running the tests from the master branch of Birdy. I don't know if it's something on my part or if it's an expected/known behavior, being new to Birdy's development. |
@f-PLT I'm tagging @cehbrecht as he's much more capable of tackling these errors than I. |
@f-PLT code looks good to me. I was checking with a local instance of emu and tests all passed. Thanks for the good work :) |
I've added an explicit test using the Emu branch with the new translation process and everything is working fine. @f-PLT @cehbrecht At the moment, Options:
Thoughts ? |
I suggest we merge this PR as is, and then open a new ticket to review settings access and modifications in general. |
I would vote for the first two options. |
I agree. |
Do not merge before a OWSLib version containing language support has been released. (Currently only on master branch)
Has been tested using local copy of OWSLib library taken from it's master branch.
Before merging
requirements.txt
,environment.yml
and where also necessaryOverview
This PR applies changes to integrate language support being integrated in PyWPS and OWSLib geopython/OWSLib#654
Changes:
WPSClient
andBirdyCLI
classesWPSClient
language property.