Skip to content
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

Support serving multiple libraries by one server #47

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

robooo
Copy link

@robooo robooo commented Feb 10, 2018

No description provided.

Changes has been made to support serving a list of libraries:
robotframework#19
serving multiple classes by one single server
Changes has been made to support serving a list of libraries:
robotframework#19
update example for change of serving multiple libraries
@loumaran
Copy link

We are using this adaptions now for more than one year. It is absolut stable and we recognized no problems. Is there a possibility that this feature will be merged into master in the near future? And if not, are there any doubts about this integration?
Can we provide any additional information or help if needed?

Thanks in advance,
Mario

Copy link

@srinivaschavan srinivaschavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robooo , these code changes look good. I ended up making similar changes before looking at your PR.

For completeness sake, I think the docs should be updated to indicate that library should be a list. Also, I feel that we should keep backward compatibility support for library not being a list.

@Erich-McMillan
Copy link

@srinivaschavan @robooo @loumaran would be awesome to see this merged in. Can we get an update here?

@srinivaschavan I'm happy to update documentation if that's what's needed to move this along. That would be primarily the readme in this repo? Perhaps adding a Multiple library support section? Since I don't have write access to the forked repo here, can we merge this then I can create a separate PR to update the docs.

@srinivaschavan
Copy link

@Erich-McMillan , yes updating the README should suffice. However, the real concern with this PR is that it is not backwards compatible. IMO, we should handle the case where users have not specified a list of libraries.

@Erich-McMillan
Copy link

@Erich-McMillan , yes updating the README should suffice. However, the real concern with this PR is that it is not backwards compatible. IMO, we should handle the case where users have not specified a list of libraries.

Agreed, best to allow users to continue passing in a single library without putting it into a list. Would the best path forward be to have the constructor check the type of library to see if it is a list then act accordingly?

Also if no feedback from previous authors, I can create a new PR to introduce those changes alongside docs.

@@ -70,7 +70,8 @@ def __init__(self, library, host='127.0.0.1', port=8270, port_file=None,
``Stop Remote Server`` keyword and
``stop_remote_server`` XML-RPC method.
"""
self._library = RemoteLibraryFactory(library)
self._library = [RemoteLibraryFactory(library_)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srinivaschavan to maintain backward compatibility could this be a solution?

Suggested change
self._library = [RemoteLibraryFactory(library_)
if isinstance(libraries, list):
self._libraries = [RemoteLibraryFactory(library_)
for library_ in libraries]
else:
self._libraries = [RemoteLibraryFactory(libraries)]

Though not sure if checking isinstance is the most pythonic, maybe checking to see if the item supports __iter__ would be more robust?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this solutions works. Best if you create a new PR with these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants