Implement ActionQueue for batching actions in OverkizClient

[iMicknl]

Fixes #1865

[Copilot]

Pull request overview

This PR implements an optional ActionQueue feature for the OverkizClient to address Overkiz API concurrency limits by batching multiple action executions into single API calls within a configurable time window.

Key Changes:

• Introduces ActionQueue and QueuedExecution classes for batching action executions
• Adds optional queue configuration to OverkizClient (disabled by default for backward compatibility)
• Modifies execute_action_group to support both immediate and queued execution modes

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 19 comments.

Show a summary per file

File	Description
pyoverkiz/action_queue.py	New module implementing ActionQueue for batching actions and QueuedExecution for awaitable results
pyoverkiz/client.py	Integrates ActionQueue with new constructor parameters, modified execute_action_group method, and helper methods for queue management
pyoverkiz/models.py	Expands Command.name type to accept str or OverkizCommand
tests/test_action_queue.py	Comprehensive unit tests for ActionQueue functionality including batching, flushing, and error handling
tests/test_client_queue_integration.py	Integration tests validating queue behavior with OverkizClient
test_queue_example.py	Demonstration script showing queue feature usage patterns

Comments suppressed due to low confidence (1)

pyoverkiz/models.py:479

• The Command.name field type has been changed from OverkizCommand to str | OverkizCommand, but the init method still expects only OverkizCommand. This inconsistency could lead to type checking issues. The init signature should be updated to accept str | OverkizCommand to match the field type definition, or the field type should be reverted if string support is not needed.

    name: str | OverkizCommand
    parameters: list[str | int | float | OverkizCommandParam] | None

    def __init__(
        self,
        name: OverkizCommand,

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[iMicknl]

@copilot please look extensively at all review comments and address them

[Copilot]

@iMicknl I've opened a new pull request, #1876, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

After propagating the exception to all waiters, the exception is re-raised. However, this might cause issues because if flush() or shutdown() calls this method, the exception will bubble up and potentially interrupt the cleanup process. Consider whether re-raising is necessary here, or if it should be handled differently when called from shutdown/flush contexts to ensure graceful cleanup.

[Copilot]

The error message should include the actual value that was provided to help with debugging. Consider changing to "action_queue_max_actions must be at least 1, got {action_queue_max_actions}".

Suggested change

	raise ValueError("action_queue_max_actions must be at least 1")
	raise ValueError(
	f"action_queue_max_actions must be at least 1, got {action_queue_max_actions}"
	)

[Copilot]

The docstring is missing documentation for the credentials and verify_ssl parameters. These parameters should be documented to provide complete information about all constructor parameters.

[Copilot]

The error message should include the actual value that was provided to help with debugging. Consider changing to "action_queue_delay must be positive, got {action_queue_delay}".

Suggested change

	raise ValueError("action_queue_delay must be positive")
	raise ValueError(f"action_queue_delay must be positive, got {action_queue_delay}")

[Copilot]

Executing a batch while holding the lock at line 121 could cause long lock hold times and potential deadlocks. The batch execution involves an async network call via _executor, which could take significant time. This blocks all other queue operations during execution.

Consider refactoring to execute both batches outside the lock. You could collect both batches to execute and then execute them sequentially after releasing the lock.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The error messages should specify the actual invalid values that were provided to help users debug configuration issues. For example, "action_queue_delay must be positive, got {action_queue_delay}" would be more helpful.

Suggested change

	raise ValueError("action_queue_delay must be positive")
	if action_queue_max_actions < 1:
	raise ValueError("action_queue_max_actions must be at least 1")
	raise ValueError(
	f"action_queue_delay must be positive, got {action_queue_delay!r}"
	)
	if action_queue_max_actions < 1:
	raise ValueError(
	"action_queue_max_actions must be at least 1, "
	f"got {action_queue_max_actions!r}"
	)

[Copilot]

Creating asyncio.Future() in init (a synchronous method) can cause issues. The Future should be created lazily when first needed in an async context. Consider creating the future in the add() method the first time it's called, or switching to using asyncio.Event and managing the result separately.

Suggested change

	self._future: asyncio.Future[str] = asyncio.Future()
	def set_result(self, exec_id: str) -> None:
	"""Set the execution ID result."""
	if not self._future.done():
	self._future.set_result(exec_id)
	def set_exception(self, exception: BaseException) -> None:
	"""Set an exception if the batch execution failed."""
	if not self._future.done():
	self._future.set_exception(exception)
	def is_done(self) -> bool:
	"""Check if the execution has completed (either with result or exception)."""
	return self._future.done()
	def __await__(self):
	"""Make this awaitable."""
	return self._future.__await__()
	self._future: asyncio.Future[str] | None = None
	def _ensure_future(self) -> asyncio.Future[str]:
	"""Create the underlying future lazily, bound to the running event loop."""
	if self._future is None:
	loop = asyncio.get_running_loop()
	self._future = loop.create_future()
	return self._future
	def set_result(self, exec_id: str) -> None:
	"""Set the execution ID result."""
	future = self._ensure_future()
	if not future.done():
	future.set_result(exec_id)
	def set_exception(self, exception: BaseException) -> None:
	"""Set an exception if the batch execution failed."""
	future = self._ensure_future()
	if not future.done():
	future.set_exception(exception)
	def is_done(self) -> bool:
	"""Check if the execution has completed (either with result or exception)."""
	return self._future.done() if self._future is not None else False
	def __await__(self):
	"""Make this awaitable."""
	return self._ensure_future().__await__()

[Copilot]

While the documentation acknowledges this is a best-effort read, accessing self._pending_actions without the lock in a concurrent environment can lead to subtle bugs in Python due to list operations not being atomic. Consider acquiring the lock briefly to get an accurate count, or document more clearly that this method may return stale data and should not be relied upon for critical logic.