search

oxan / esphome-stream-server

Stream server (serial-to-wifi bridge) for ESPHome
Other
190 stars 76 forks source link

Introduce basic CI pipeline and apply ESPHome code style #13

Closed syssi closed 2 years ago

syssi commented 2 years ago

Please squash the commits before your merge this PR. I didn't squash it to simplify the review.

oxan commented 2 years ago

Hi! Thanks for your contribution.

However, I'm not going to be merging this. While I agree a CI pipeline would be nice, I'm not too happy with the way this is implemented. Cloning the dev version of ESPHome and copying in the component is a very hacky way to get the CI suite from ESPHome to run on an external component, and makes CI sensitive to changes in ESPHome: both because these scripts are considered internal at ESPHome, so they can break compatibility at any time; and because it runs CI on the full codebase, so if some component in ESPHome fails, CI here will fail as well. Using the dev version of ESPHome for linting and the released version for compile-testing could cause problems as well. I'm also not sure why you copied the clang-format config, but not the clang-tidy one (so they can get desynchronized).

But aside from the implementation, I'm not convinced I actually want to apply the ESPHome code style here. There's a few things in there that I don't necessarily agree with, and some things I actively dislike.

I've actually been thinking about CI (and other developer experience improvements) for external components lately, and I intent to refactor the ESPHome CI scripts to support external components, be more modular and to provide a GitHub Action (somewhat similar to the HA hassfest action). That avoids duplication of code and configs and things getting out-of-sync, allows components to pick only the parts they want, can better support versioning, and seems way less likely to break in general.