rename unclear expressions #695

stan-dot · 2024-10-29T12:34:39Z

No description provided.

stan-dot · 2024-10-29T12:35:56Z

the error is just the Task vs TaskRequest

'TaskRequest': {
? -------

        'Task': {
            'additionalProperties': False,

            'description': 'Task that will run a plan, different from the WorkerTask',

? -------------------------------

            'description': 'Task that will run a plan',
            'properties': {
                'name': {

codecov · 2024-10-29T12:41:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (00572da) to head (ad46d8a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #695   +/-   ##
=======================================
  Coverage   92.32%   92.33%           
=======================================
  Files          35       35           
  Lines        1799     1800    +1     
=======================================
+ Hits         1661     1662    +1     
  Misses        138      138

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

callumforrester

Thank you for the PR @stan-dot; there are some great suggestions here that will definitely improve the code. I noticed a few suggestions that seem to reflect personal coding preferences or may stem from unfamiliarity with some Python-specific practices. I’ve offered some gentle feedback on those points to clarify the current approach and rationale.

callumforrester · 2024-10-29T12:51:21Z

src/blueapi/client/client.py

@@ -40,17 +40,16 @@ def __init__(
    @classmethod
    def from_config(cls, config: ApplicationConfig) -> "BlueapiClient":
        rest = BlueapiRestClient(config.api)
+        events: EventBusClient | None = None


Should: This is an extra unnecessary assignment compared to the else statement, please restore

callumforrester · 2024-10-29T12:52:09Z

src/blueapi/client/client.py

+            stomp_client = StompClient.for_broker(
                broker=Broker(
                    host=config.stomp.host,
                    port=config.stomp.port,
                    auth=config.stomp.auth,
                )
            )
-            events = EventBusClient(template)
-        else:
-            events = None
+            events = EventBusClient(stomp_client)


Could: Wrap this:

events = EventBusClient(StompClient.for_broker(...))

why wrap this?

Not a strong preference, just suggest giving it a go and seeing if it reads better

for me it's clearer when the two assignments are separate

callumforrester · 2024-10-29T12:53:58Z

src/blueapi/client/client.py

@@ -197,7 +196,7 @@ def run_task(

        complete: Future[WorkerEvent] = Future()

-        def inner_on_event(event: AnyEvent, ctx: MessageContext) -> None:
+        def _generic_event_callback(event: AnyEvent, ctx: MessageContext) -> None:


Should: I'm happy to accept that inner_on_event is not a very clear name, but I don't think this either. Also you don't need an underscore at the front, it is already private by virtue of being a nested function. How about just on_event or for_each_event?

Also you don't need an underscore at the front, it is already private by virtue of being a nested function

yes but for redundancy underscore is more signal

on_event is good

callumforrester · 2024-10-29T12:54:51Z

src/blueapi/client/event_bus.py

@@ -17,23 +17,23 @@ def __init__(self, message: str) -> None:


 class EventBusClient:
-    app: StompClient
+    facade: StompClient


Should: Not quite, if anything, the outer class is a facade. Could make this private and call it _client, _inner_client or _stomp_client?

_stomp_client the best

src/blueapi/client/rest.py

callumforrester · 2024-10-29T13:03:07Z

src/blueapi/service/runner.py

@@ -141,7 +140,7 @@ def _rpc(
    **kwargs: Any,
 ) -> T:
    mod = import_module(module_name)
-    func: Callable[P, T] = _validate_function(
+    func: Callable[..., T] = _validate_function(


Should: This is less type safe since mypy/pyright won't check the function parameters, see docs, please revert.

there was an error in the IDE

What was the error? Did you understand what was causing it?

the typing system was not happy, can look into what exactly it is

callumforrester · 2024-10-29T13:03:54Z

src/blueapi/worker/task.py

    """
-    Task that will run a plan
+    Task that will run a plan, different from the WorkerTask


Should: I don't mind this addition as long as you explain how it is different from the WorkerTask

callumforrester · 2024-10-29T13:04:42Z

src/blueapi/worker/task.py

@@ -10,9 +10,9 @@
 LOGGER = logging.getLogger(__name__)


-class Task(BlueapiBaseModel):
+class TaskRequest(BlueapiBaseModel):


This one I am unsure about, this class is definitely more than a request since it has a method to actually run the task, but maybe it shouldn't? Maybe they should be separated? Thoughts welcome.

definitely confusing when we have WorkerTask

Agreed, I think do_task() should be extracted somewhere else, could you make a separate issue for that please?

callumforrester · 2024-10-29T13:05:52Z

src/blueapi/worker/task_worker.py

+        # Define filters for each status
+        status_filter = {
+            TaskStatusEnum.RUNNING: lambda task: not task.is_pending
+            and not task.is_complete,  # noqa: E501
+            TaskStatusEnum.PENDING: lambda task: task.is_pending,
+            TaskStatusEnum.COMPLETE: lambda task: task.is_complete,
+        }.get(status, lambda task: False)  # Default to a filter that returns no tasks
+
+        # Apply the filter to _tasks values
+        return [task for task in self._tasks.values() if status_filter(task)]


Should: I don't think this any clearer, please revert? Alternatively see if it's any better with match statements

I think it's clearer. therefore it's a Pareto improvement in aggregate perception of clarity

callumforrester · 2024-10-29T13:11:39Z

src/blueapi/service/interface.py

+    if stomp_config is None:
+        return None


Should: Please revert this approach

I'm picking this out as an example but there are many others in this PR where you achieve exhaustiveness via early returns rather than explicit flow control i.e.

def foo() -> int: if bar: return 2 - else: - return 5 + return 5

This is quite an opinionated change and contrary to the approach used so far in the codebase. In reality the above diff makes no functional change and, in my view, is just less readable because it removes context and connecting words.

I imagine there's a fair bit of literature about this arguing for different patterns and paradigms but it would be a bit bike sheddy to go through it all. I would suggest just trying to be consistent with the codebase as you found it. If you really find the pattern that's there unreadable, please explain why and we can try to figure out how to make things better.

you achieve exhaustiveness via early returns rather than explicit flow control

yes this is on purpose

but ok if this is in the codebase it makes no sense to fight it

but

In reality the above diff makes no functional change

that's the defintion of refactoring

connecting words

different logical branches are decoupled, they don't need connection

The goal isn't to write as few words as possible it's just that

if foo: return 1 else: return 2

reads better than

if foo: return 1 return 2

Because it translates more directly into natural language

reads better than

that's subjective

'if-else' assumes it's 2 branches. 'early return' reads the same when the number of branches rises, and is more appropriate than a switch statement in cases when the branches aren't very cleanly different based on typeof someVariable.

`if-else', if more branches grow becomes a hard structure to track.

furthermore

translates more directly into natural language

it's not obvious whether that is good or bad...

https://www.youtube.com/watch?v=EumXak7TyQ0

https://developer20.com/elseless/

As the number of conditions and branches increases, the code becomes harder to understand and maintain.

This is very much a straw-man argument.

if foo: print("foo") - else: - print("bar") + print("bar")

The number of branches here doesn't change just by removing the word "else", it's just that the existence of one is now implicit rather than clearly specified. Adding more words does not always make something harder to understand.

I clearly didn't state my argument well enough.

trying again ...

for any location in code where business logic has multiple branches, that must be reflected in the code.

patterns are many: switch statements, wrapping callbacks in a dictionary (like for filter_by_status), if + early return, if-else.

Rust uses switch-y pattern matching very often.

in Python when we have 10 branches we'll almost always go for switch, which would feel cumbersome if there was just 2.

early return, which is my favourite isn't as great if there are 10 branches, visually, scales better than if-else

with early return when I read a function I keep track of branches and possible end-states of the function.
Every if with early return opens one possible end state and then closes it in the same place.

if a: do_x() return if b: do_y() do_z()

with if-else it's easier to get confused as each layer cuts in 2 the possibility space and as a reader I now need to track 2^n possibilities and see if the writer accounted correctly for all of them.

therefore the 'as the number of branches increases' refers to both lifetime of the codebase and the distribution of business logic instances in complexity from the start.

early return is more futureproof and a habit to avoid making if-else where some of the possiblities aren't addressed

branch has been reset since

stan-dot added enhancement New feature or request python Pull requests that update Python code labels Oct 29, 2024

stan-dot requested review from ZohebShaikh and callumforrester October 29, 2024 12:34

stan-dot self-assigned this Oct 29, 2024

stan-dot force-pushed the rectification-of-names branch from 0b2a78d to fd0787c Compare October 29, 2024 12:36

callumforrester previously requested changes Oct 29, 2024

View reviewed changes

stan-dot marked this pull request as ready for review October 29, 2024 13:14

fresh take on the change of names

ad46d8a

stan-dot force-pushed the rectification-of-names branch from fd0787c to ad46d8a Compare November 11, 2024 10:22

stan-dot requested a review from callumforrester November 11, 2024 10:41

callumforrester approved these changes Nov 11, 2024

View reviewed changes

stan-dot merged commit 0f210d5 into main Nov 11, 2024
28 checks passed

stan-dot deleted the rectification-of-names branch November 11, 2024 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename unclear expressions #695

rename unclear expressions #695

stan-dot commented Oct 29, 2024

stan-dot commented Oct 29, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

callumforrester left a comment

callumforrester Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 30, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 30, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 29, 2024

callumforrester Oct 29, 2024

stan-dot Oct 30, 2024

callumforrester Nov 11, 2024

stan-dot Nov 11, 2024

rename unclear expressions #695

rename unclear expressions #695

Conversation

stan-dot commented Oct 29, 2024

stan-dot commented Oct 29, 2024

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

callumforrester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 •

edited

Loading