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

Make Go2RtcRestClient.validate_server_version a classmethod #17

Conversation

emontnemery
Copy link
Contributor

Proposed Changes

Make Go2RtcRestClient.validate_server_version a classmethod; the method will be called before a client is created to validate the server

Related Issues

(Github link to related issues or pull requests)

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (48fdb78) to head (5540ea2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   97.85%   97.87%   +0.02%     
==========================================
  Files           7        7              
  Lines         280      283       +3     
  Branches       46       48       +2     
==========================================
+ Hits          274      277       +3     
  Misses          4        4              
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emontnemery emontnemery force-pushed the validate_server_version_classmethod branch from 4bd3b28 to 776173f Compare October 29, 2024 16:07
@emontnemery emontnemery added the enhancement Enhancement of the code, not introducing new features. label Oct 29, 2024
async def validate_server_version(self) -> None:
async def validate_server_version(
cls, websession: ClientSession, server_url: str
) -> None:
Copy link
Contributor

@MartinHjelmare MartinHjelmare Oct 30, 2024

Choose a reason for hiding this comment

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

Is this really more convenient? Won't this just mean we have to pass the session and url twice to two calls instead of passing them once to the first of two calls, when validating the server version and creating a client? We also just save two instance creation calls out of four internally compared to creating the client from the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same discussion with Robert. From the user's point of view, I think it makes sense to make it a class method; there's no need for a client if the server has the wrong version.
From the current implementation point of view it's as you say currently not useful since the implementation creates the client objects.
At the same time, the client objects are very lightweight and cost very little to create.

@emontnemery
Copy link
Contributor Author

I'm closing this since both @MartinHjelmare and @edenhaus think it's a bad idea

@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhancement of the code, not introducing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants