esphome / feature-requests

ESPHome Feature Request Tracker
https://esphome.io/
417 stars 26 forks source link

Add way to abort an action list #1769

Open sybrenstuvel opened 2 years ago

sybrenstuvel commented 2 years ago

Describe the problem you have/What new integration you would like

I want to have the ability to "break out of" or abort an action list. This will aid in flattening conditional code by un-nesting, i.e. it will lower the cognitive complexity of the code.

Please describe your use case for this integration and alternatives you've tried:

I have YAML like this:

esphome:
  on_boot:
    priority: 100
    then:
      - if:
          condition:
            lambda: "return esp_sleep_get_wakeup_cause() != ESP_SLEEP_WAKEUP_TOUCHPAD;"
          then:
            - logger.log: "Did not wake up because of touchpad, ignoring this boot"
          else:
            - logger.log: "Woke up from touch, going to wait for API & send event"
            - more code here to actually do stuff on wake-up-on-touch.

This approach works, but has the downside that all the subsequent code would be nested under the else. As a result, a consistent application of this pattern would make the execution order "read" from top left to bottom right, nesting ever deeper.

Flipping the condition would nest it under the inner then, which is at the same nesting level, so doesn't solve the actual issue.

The goal of this feature request is to have something like this:

# initial lines same as above.
    then:
      - if:
          condition:
            lambda: "return esp_sleep_get_wakeup_cause() != ESP_SLEEP_WAKEUP_TOUCHPAD;"
          then:
            - logger.log: "Did not wake up because of touchpad, ignoring this boot"
            - abort:
      - logger.log: "Woke up from touch, going to wait for API & send event"
      - more code here to actually do stuff on wake-up-on-touch.

As you can see, the code flow now is directly under the top-level then: instead of nested.

Additional context Translated to C++, the above two blocks of code would loosely map to:

void currently_possible() {
  if (esp_sleep_get_wakeup_cause() == ESP_SLEEP_WAKEUP_TOUCHPAD) {
    // All of this function's behaviour inside this condition.
  }
}
void requested() {
  if (esp_sleep_get_wakeup_cause() != ESP_SLEEP_WAKEUP_TOUCHPAD) return;
  // All of this function's behaviour outside the condition.
}

Repeating the currently_possible() approach will cause deeper and deeper nesting, and typically makes it harder to understand the main flow as each condition needs to be inspected carefully. Repeating the requested() approach will just push the main behaviour down, and the main flow will remain top-to-bottom.

mmakaay commented 2 years ago

Having an abort does not make the code cleaner IMO. If I'm scanning that code as a first time reader, I might be looking for something specific and skip to the logger.log line when I see that the if condition has no relation to my search. With the code all at the top level, it gives this impression that it's a bunch of actions that is executed from A to B.

With abort, which would be closely related to return and break in other languages, the next problem will be that there will be a need for specifying the number of nest levels that should be aborted. Is it only for the current nest level, is it for all nesting levels, or should one be able with three nesting levels to break out of 2 of these, using abort: 2 or so?

One way in programming languages to get to cleaner code in these situations, is to chop up the code into functional blocks. And that is how I cleanup my YAML for ESPHome myself, by making use of the script: feature. For example with your case, you could cleanup the code like this:

script:
  - id: handle_wake_up_on_touch
    then:
      - logger.log: "Woke up from touch, going to wait for API & send event"
      - more code here to actually do stuff on wake-up-on-touch.

esphome:
  on_boot:
    priority: 100
    then:
      - if:
          condition:
            lambda: "return esp_sleep_get_wakeup_cause() == ESP_SLEEP_WAKEUP_TOUCHPAD;"
          then:
            - script.execute: handle_wake_up_on_touch
          else:
            - logger.log: "Did not wake up because of touchpad, ignoring this boot"

Also reversed the logic, because in general == cases are easier to process for readers than != cases. This could be even go a step further, by removing the low level details from the high level on_boot: trigger:

script:
  - id: check_for_wake_up_on_touch
    then:
      if:
        condition:
          lambda: "return esp_sleep_get_wakeup_cause() == ESP_SLEEP_WAKEUP_TOUCHPAD;"
        then:
          script.execute: handle_wake_up_on_touch
        else:
          logger.log: "Did not wake up because of touchpad, ignoring this boot"

  - id: handle_wake_up_on_touch
    then:
      - logger.log: "Woke up from touch, going to wait for API & send event"
      - more code here to actually do stuff on wake-up-on-touch.

esphome:
  on_boot:
    priority: 100
    then:
      script.execute: check_for_wake_up_on_touch

Another small cleanup I did in this version, is to not use a list for action lists, when there is only one action in it. I find it easier on the eyes this way. This last version is probably where I would end up when cleaning up the code. Now a reader can dig into the code from the high level to the low level code, and nesting levels have been cleared.

Would you agree that this is also a feasible way of getting rid of excessive code nesting?

sybrenstuvel commented 2 years ago

Having an abort does not make the code cleaner IMO. If I'm scanning that code as a first time reader, I might be looking for something specific and skip to the logger.log line when I see that the if condition has no relation to my search. With the code all at the top level, it gives this impression that it's a bunch of actions that is executed from A to B.

Not sure what you mean here. If you're not reading all the code and looking for something you suspect, then any code that doesn't adhere to your expectations is going to give you trouble.

With abort, which would be closely related to return and break in other languages, the next problem will be that there will be a need for specifying the number of nest levels that should be aborted. Is it only for the current nest level, is it for all nesting levels, or should one be able with three nesting levels to break out of 2 of these, using abort: 2 or so?

That's something we should address, indeed. Personally I'd be fine if it were to abort the entire top-level thing, i.e. the entire on_boot: then list of actions. In this case, anyway.

One way in programming languages to get to cleaner code in these situations, is to chop up the code into functional blocks.

I know. Some context, I've been writing software for the past 30 years, and taught people software design as well. This is me: Scripting for Artists on YouTube πŸ˜„

And that is how I cleanup my YAML for ESPHome myself, by making use of the script: feature. For example with your case, you could cleanup the code like this:

Yeah, that'll certainly be useful to split up some more complex cases. Still, even when using this as some form of functions, it's missing a way to return early.

Also reversed the logic, because in general == cases are easier to process for readers than != cases.

This is something I strongly disagree with. Code like the following is abundant in so many projects, but having the "error check" and "error handling" separated with lots of "happy flow" code is IMO much worse to read than a negation:

int function(int someArg) {
  if (someArg >= 0) {
    // multiple lines
    // of happy-flow
    // code here
  } else {
    // Handle error case and return error status.
    return -1;
  }

  return 0;
}

while I would write this as:

int function(int someArg) {
  if (someArg < 0) {
    // Handle error case and return error status.
    return -1;
  }

  // multiple lines
  // of happy-flow
  // code here
  return 0;
}

Having the code in the "if it's wrong, abort ASAP" style generally makes things much easier.

Another small cleanup I did in this version, is to not use a list for action lists, when there is only one action in it.

Yup, that helps too, nice.

Would you agree that this is also a feasible way of getting rid of excessive code nesting?

It helps, for sure. Then again, a long list of scripts that can only be identified by name and have no other way of organising things can also get messy. Of course in such situations I could create lambdas and just write in C++. Personally I'd still be missing a way to break out of code once I know it should stop.

mmakaay commented 2 years ago

My context is almost 40 years of programming, so that trumps your 30 πŸ˜„

If you're not reading all the code and looking for something you suspect, then any code that doesn't adhere to your expectations is going to give you trouble.

One does not have to read all the code, when you have code cleanly separated in high level policy and low level implementation details. One does not need to read all code from A to B, when one's trying to find a specific piece of code or understand a system. With good policy vs detail implementation, navigating the code becomes a lot easier, because one can use the high level code as a kind of index and skip the low level code that isn't relevant at the time of reading.

When taking your example code, this would (in extrema) translate to something like:

The top level policy function. One can see what it does, without knowing the internals of the called functions: "it takes someArg, if the arg is valid the happy flow is executed and 0 is returned, otherwise the error case is excuted and -1 is returned":

int function(int someArg) {
  if (isValidSomeArg(someArg)) {
    doHappyFlow();
    return 0;
  }
  handleErrorCase();
  return -1;
}

When people are interested in only one of the above policy building blocks (e.g. "I want to know why my someArg is rejected"), then they can jump to the appropriate function and see what it does at the detail level:

bool isValidSomeArg(int someArg) {
    return someArg >= 0;
}

void doHappyFlow() {
  // multiple lines
  // of happy-flow
  // code here
}

void handleErrorCase() {
  // logging
  // displaying
  // whatever further
}

About reversing the != with ==: In cases like the YAML code that I wrote, it is often easier to read. Of course there are many examples in which this would not apply. Your example demonstrates this, and in a C-family language, I would probably handle it like that too.

It's easy to get into really big discussions following all this, and I propose not to do that. How to cleanly organize code can easily be more controversial than "tabs versus spaces" (spaces of course ;-)

So back to the idea at hand: implement abort

When I said that we'd quickly run into the need for abort: 2 and such, I was actually trying to communicate in what way I'm not a fan of such implementation. It binds code to its code depth. This can result in fun situations when the code moves to a different depth level, without updating the abort accordingly. It's basically a weakly specified goto statement, and we all know they make Dijkstra cry.

Having said all that, I am in no way authoritative over what features are added to ESPHome and other reviewers might have different opinions than me. I'm perfectly okay with it if this finds its way into ESPHome. I'll just not be using it myself.

Some considerations in case this gets implemented:

ssieb commented 2 years ago

I'm happy to have something like this. I would suggest break: or return: or maybe even both. break: could exit the current sub-list and return: would exit the entire action script or top-level action list. For a script, I just realized from @mmakaay now that you could use the script.stop: action to stop the current script.

mmakaay commented 2 years ago

A few thoughts on that:

The problem with break: is that it is a well known keyword for jumping out of switch and for/while loops in code. Here, it would be applied to an if: statement, which is not in line with what people might already know.

Additionally, for people that don't know break from other languages, stop: would likely be a lot clearer. The terminology is closer to the actual functionality that the user is after: "stop running this script".

Using return: from an action list doesn't feel right. It's not within the context of a script, so there's not really something to return from. I can see a use of return: within the context of a script, as long as it can be used to actually return data to be consumed by a caller (return: 1234).

Thinking about it, is there actually a good use case for ending the current action list, when some construct exists that ends the top level action execution? Let's say that we've got break: to end the current sublist, can you provide some code that would use that? For me, this would lead to a superfluous break::

      - if:
          condition:
            - something: ...
          then:
            - do: something
            - here: and then
            - break:            
      - and: the code
      - would: continue here

or unreachable code

      - if:
          condition:
            - something: ...
          then:
            - do: something
            - here: and then
            - break:
            - and: here might be
            - some: code that can
            - never: be reached
      - and: the code
      - would: continue here
sybrenstuvel commented 2 years ago

My context is almost 40 years of programming, so that trumps your 30 πŸ˜„

There's always an older fart ;-)

int function(int someArg) {
  if (isValidSomeArg(someArg)) {
    doHappyFlow();
    return 0;
  }
  handleErrorCase();
  return -1;
}

That certainly wouldn't get past a code review if you let me have my way. In any case, this discussion shows how different people approach coding in different ways, and I think it would be very nice if ESPHome would have some more flow control.

I'm quite fond of how Go handles break. It breaks out of the inner-most loop, but if you want you can use labels to break out of outer loops. The label is attached to the for, while etc. statement.

I agree with the awkwardness of having break or return in YAML not exactly matching with their regular counterparts. This is also why I chose something different; stop would be fine by me as well.

Unreachable code can indeed occur, but can be detected & warned about.

mmakaay commented 2 years ago

Unreachable code is a given with constructions like this. Easy to report on yes. But the point was wether or not we'd need a construction to break out of a single nesting level when handling if: constructions. That in reaction to the proposal:

break: could exit the current sub-list and return: would exit the entire action script or top-level action list.

There are conditional branching and loops, and in the discussion they are getting mingled a bit. The issues started about "if", but now includes loops as well. I'm fine with that, but it broadens the scope a bit to multiple forms of flow control changes.