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

added second websocket to get the Challenge ws. #223

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Leicas
Copy link

@Leicas Leicas commented Nov 16, 2024

No description provided.

Copy link

@gitpack-ai gitpack-ai bot left a comment

Choose a reason for hiding this comment

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

Code Review for this PR

The changes introduce new functionality for handling a new websocket (websocket2) as well as an endpoint related to Automation Challenges. These additions are integrated into the existing methods and create new helper methods for negotiating and managing websocket connections.

Positives:

  • Addition of automated negotiation and connection parameter retrieval for 'websocket2' which extends the capability of the API client.
  • Comprehensive debug logging has been consistently applied throughout the modifications, which assists in tracing the flow and pinpointing issues.

Areas of Improvement:

  • Inline comments explaining non-obvious decisions should be enhanced for better context understanding.
  • The redundancy in websocket2 initialization within the '_async_post_init' method needs to be evaluated for optimization and to prevent future maintenance challenges.

Comment on lines +232 to +234
if endpoint.startswith(AUTOMATION_CHALLENGE_ENDPOINT):
# remove Ocp-Apim-Subscription-Key header to avoid 401 error
kwargs["headers"].pop("Ocp-Apim-Subscription-Key", None)
Copy link

Choose a reason for hiding this comment

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

The logic for removing the 'Ocp-Apim-Subscription-Key' header should be documented with a rationale or alternatively, a reference to a relevant section of the API documentation if available. This will help future developers understand the necessity of this operation in the context of API interactions.

Suggested change
if endpoint.startswith(AUTOMATION_CHALLENGE_ENDPOINT):
# remove Ocp-Apim-Subscription-Key header to avoid 401 error
kwargs["headers"].pop("Ocp-Apim-Subscription-Key", None)
# remove Ocp-Apim-Subscription-Key header based on API requirements to avoid 401 error

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.

2 participants