Skip to content

Conversation

@ayeletstarkware
Copy link
Contributor

No description provided.

@ayeletstarkware ayeletstarkware marked this pull request as ready for review November 25, 2025 15:21
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

ayeletstarkware commented Nov 25, 2025

@ayeletstarkware ayeletstarkware force-pushed the ayelet/echonet/l1_client/refactor branch from 7da3abf to 55d7bbb Compare November 25, 2025 15:28
Copy link
Collaborator

@matanl-starkware matanl-starkware left a comment

Choose a reason for hiding this comment

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

@matanl-starkware reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware)


echonet/l1_client.py line 41 at r2 (raw file):

        self,
        api_key: str,
        logger: logging.Logger,

Usually, it's the responsibility of the class to create its own logger.
If you want, it's possible to supply a logger "name" externally, but that's also not useful most of the times.

Code quote (i):

logger: logging.Logger,

Code snippet (ii):

self.logger = logging.Logger('L1Client')

echonet/l1_client.py line 55 at r2 (raw file):

        self,
        request_func: Callable[[], Dict],
        method_name_for_logs: str,

Consider getting this from the call stack

Suggestion:

import inspect

def caller():
    callee()

def callee():
    caller_frame = inspect.currentframe().f_back
    print(caller_frame.f_code.co_name)

caller()  # prints: caller

echonet/l1_client.py line 105 at r2 (raw file):

            response = requests.post(self.rpc_url, json=payload, timeout=self.timeout)
            response.raise_for_status()
            return response.json()

Move the error handling and response parsing to the common retries function.

Suggestion:

request_func = lambda: requests.post(self.rpc_url, json=payload, timeout=self.timeout)

echonet/l1_client.py line 106 at r2 (raw file):

            response.raise_for_status()
            return response.json()

Another alternative:
Leave the "timeout" handling to the retries function.

Code snippet:

request_func = functools.partial(requests.post, self.rpc_url, json=payload)

...
response = request_func(timeout=self.timeout)

@ayeletstarkware ayeletstarkware force-pushed the ayelet/echonet/l1_client/refactor branch from 55d7bbb to 739b502 Compare November 26, 2025 09:03
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matanl-starkware)


echonet/l1_client.py line 41 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Usually, it's the responsibility of the class to create its own logger.
If you want, it's possible to supply a logger "name" externally, but that's also not useful most of the times.

Done.


echonet/l1_client.py line 55 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Consider getting this from the call stack

Cool!


echonet/l1_client.py line 105 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Move the error handling and response parsing to the common retries function.

used functools


echonet/l1_client.py line 106 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Another alternative:
Leave the "timeout" handling to the retries function.

Done.

Copy link
Collaborator

@matanl-starkware matanl-starkware left a comment

Choose a reason for hiding this comment

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

@matanl-starkware reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

@ayeletstarkware ayeletstarkware added this pull request to the merge queue Nov 27, 2025
Merged via the queue into main-v0.14.1 with commit 2b23390 Nov 27, 2025
17 checks passed
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