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 #82

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

Conversation

Erich-McMillan
Copy link

@Erich-McMillan Erich-McMillan commented Dec 14, 2022

Summary

Addresses concerns from PR #47 , by

  1. Allowing backward compatibility with previous interface which only supported a single class while still supporting a list of classes.
  2. Updating documentation to include section on serving multiple libraries with a single server.

Also updates property from _library to _libraries to improve clarity for reader.

  • new functionality
  • breaking changes
  • documentation

@Erich-McMillan
Copy link
Author

@srinivaschavan this PR supersedes #47 per your request to create a new PR, review feedback is appreciated :)

@@ -34,4 +34,4 @@ def strings_should_be_equal(self, str1, str2):


if __name__ == '__main__':
RobotRemoteServer(ExampleLibrary(), *sys.argv[1:])
RobotRemoteServer([ExampleLibrary()], *sys.argv[1:])
Copy link
Author

@Erich-McMillan Erich-McMillan Dec 14, 2022

Choose a reason for hiding this comment

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

Felt it is best to encourage/guide any new users of the remote server to use list in the example to avoid confusion going forward. Let me know if you think we should remove the list from this example.

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.

lgtm

@Erich-McMillan
Copy link
Author

@srinivaschavan, thanks :) I don't have merge permissions, any idea when this can be merged in?

@srinivaschavan
Copy link

@srinivaschavan, thanks :) I don't have merge permissions, any idea when this can be merged in?

I do not have the merge permissions either. Maybe we need approval from @pekkaklarck

@pekkaklarck
Copy link
Member

Thanks for the PR! On a super quick look it looks good. My plan is to release v1.1.1 now and then early next year look at v1.2 where this could be added.

As an APi giving libs to expose as a list sounds good. From Robot's point of view that then creates a single big library with all keywords combined.

Another thing I have been thinking is registering different libraries under different paths so that you could use e.g. http://localhost:8270/lib1 and http://localhost:8270/lib2 then taking libraries into use. In that case you'd pass libraries as a dictionary like {'lib1': Library1(), 'lib2': Library2()}. I don't know how easy registering different paths like that is, though.

@Erich-McMillan
Copy link
Author

Thanks for the PR! On a super quick look it looks good. My plan is to release v1.1.1 now and then early next year look at v1.2 where this could be added.

As an APi giving libs to expose as a list sounds good. From Robot's point of view that then creates a single big library with all keywords combined.

Another thing I have been thinking is registering different libraries under different paths so that you could use e.g. http://localhost:8270/lib1 and http://localhost:8270/lib2 then taking libraries into use. In that case you'd pass libraries as a dictionary like {'lib1': Library1(), 'lib2': Library2()}. I don't know how easy registering different paths like that is, though.

I like the idea of serving under different url paths, though I'm not sure how that would change to the high level Library Remote url import or keyword usage; would that mean using a separate remote instance per library being severed like the following example?

*** Settings ***
Library  Remote  http://localhost:8270/lib1
Library  Remote  http://localhost:8270/lib2

@pekkaklarck
Copy link
Member

Yes, you needed to take those libraries into use separately. That way you could give them separate aliases when importing using AS (or deprecated WITH NAME). I would assume that's sometimes preferred over effectively having all keywords in same library. An alternative is running multiple remote server instances, but that would require starting them separately as well.

@Erich-McMillan
Copy link
Author

In this case it may be ideal to implement a new function get_libraries/get_library (which would return a library object with get_keywords as part of the xml server? Thus the import would be Library Remote http://localhost:8270 library=lib1.

In my case I am not directly using the Remote library in .robot files but instead using it internally to another library written in python; so being able to dynamically detect+import all the libraries without user specification is desirable. Perhaps to maintain backward compatibility we could have Library Remote http://localhost:8270 import all libraries exposed by the xml server?

As for how involved implementing this will be, I'm not certain.

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.

4 participants