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

Feature Idea: Allow multiple requests of same command type to run #6

Closed
Moosems opened this issue Aug 17, 2024 · 3 comments · Fixed by #4
Closed

Feature Idea: Allow multiple requests of same command type to run #6

Moosems opened this issue Aug 17, 2024 · 3 comments · Fixed by #4
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Moosems
Copy link
Member

Moosems commented Aug 17, 2024

Feature report:

Allowing multiple requests of the same command would make the framework much more flexible and I believe this could be done without a major API change. If this is allowed, the user would have to keep track of what id's correspond to what requests for commands allowing multiple requests. They would still run concurrently but perhaps this could be changed to have flags that determine what settings are applied? To modify the simple example, it would likely look similar to this:

from time import sleep

from collegamento import (
    USER_FUNCTION,
    Request,
    Response,
    SimpleClient,
    SimpleServer,
)


def foo(server: "SimpleServer", bar: Request) -> bool:
    if bar["command"] == "test":
        return True
    return False


def main():
    commands: dict[str, USER_FUNCTION] = {"test": (foo, False)} # bool indicates whether command is only newest. Defaults to True (and not needing a bool). Maybe in the future this will use flags
    context = SimpleClient(commands)

    id1 = context.request({"command": "test"})  # Only returns an id int if the command allows multiple at once
    id2 = context.request({"command": "test"})

    sleep(1)

    # This is where the API could make one of two changes: Option one is that it doesn't and it simply returns a list of new answers:

    output: list[Response] | None = context.get_response("test")
    if len(output) < 2:
        print("Yippee! It worked!")
    else:
        print("Hmm, that didn't work", output)

    # Or, alternatively, we could request by id (raises error if id is not in system and still accepts string commands):
    output1: Response | None = context.get_response(id1)
    output2: Response | None = context.get_response(id2)
    if (output1 and output2):
        print("Yippee! It worked!")
    else:
        print("Hmm, that didn't work", output)

    context.kill_IPC()


if __name__ == "__main__":
    main()
@Moosems Moosems added the enhancement New feature or request label Aug 17, 2024
@Moosems Moosems added this to the v0.3.0 milestone Aug 17, 2024
@Moosems Moosems self-assigned this Aug 17, 2024
@Moosems
Copy link
Member Author

Moosems commented Aug 17, 2024

Relates to #5

@Moosems
Copy link
Member Author

Moosems commented Aug 17, 2024

Flags can be made a secondary issue if this goes forward

@Moosems
Copy link
Member Author

Moosems commented Aug 17, 2024

I think I like the second option more because it keeps the API much closer to what it was before. Implementation-wise we could internally treat all commands like they are multiple run commands half the time. What I mean by this is we keep a list of commands that allow multiple requests at once, the newest responses dict always corresponds to a list of Response's, and current ids has all the normal single commands along with ids as mappings. This would look as follows:

        self.multi_request_commands: list[str] = []
        self.current_ids: dict[str | int, int | str] = {} # str -> int | int -> str
        self.newest_responses: dict[str, list[Response] | Response | None] = {}

Ultimately this should try to not break past usages of the API so we have to default to old norms and keep the API similar as much as possible.

This was referenced Sep 5, 2024
@Moosems Moosems closed this as completed in #4 Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant