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

Utilize packages to reduce code duplication #6

Merged
merged 7 commits into from
Jul 26, 2024

Conversation

rohankapoorcom
Copy link
Contributor

I made a few changes for better organization in the YAML:

  • Moved the main config file inside a new bed-presence folder so that it can live alongside the new packages that it depends on.
  • Created a new bed-presence-mk1.yaml in the root of the repo that is simply a pointer to the new one (needed for dashboard import to continue to work.
  • Created a new sensor.yaml file in the bed-presence folder that uses substitutions to avoid code duplication for the left/right sensors.
  • Created a new base.yaml file in the bed-presence folder that wraps the inclusion and templating of the sensor.yaml file as well as includes the other buttons and sensors that were being created.
  • Referenced the new bed-presence/base.yaml file as a remote package from inside the new bed-presence-mk1.yaml file which still handles all of the rest of the ESPHome setup. This allows users (such as myself) to simply copy the packages block from that file and integrate it into an existing ESPHome configuration while still getting updates to the sensors defined.

Note (because of how the ESPHome remote packages work), the remote package will not work until this is merged in. You can test it from my repo meanwhile by making a quick change in bed-presence-mk1.yaml locally:

packages:
  remote_package:
    url: https://github.com/rohankapoorcom/sensor-configs
    ref: use-packages
    files: ['bed-presence/base.yaml']
    refresh: 1s

Let me know if you have any questions.

@stephenpapierski stephenpapierski self-assigned this Jul 25, 2024
@stephenpapierski
Copy link
Contributor

With the reorg, is there any reason to keep the top level bed-presence-mk1.yaml?

@rohankapoorcom
Copy link
Contributor Author

is there any reason to keep the top level bed-presence-mk1.yaml?

I believe the top level file is needed for the existing devices dashboard_import to work. Also I forgot I was supposed to turn it into a symlink to the new location, instead of putting the filename in there.

If you're not concerned in maintaining support for existing devices, we can delete the top level file and change the dashboard_import path to point to the new location.

@stephenpapierski
Copy link
Contributor

That is fair. At this point, it's only the beta devices, but it would be nice to keep those working. The Made for ESPHome project also links to that location.

What I'm considering is actually leaving the top level yaml files in the top directory, then they can still reference the base.yaml and sensor.yaml files in the underlying folder.

@rohankapoorcom
Copy link
Contributor Author

rohankapoorcom commented Jul 25, 2024

Symlinking should achieve the same result (that's how the ratgdo project does it)

@stephenpapierski
Copy link
Contributor

When I click on one of their top level symlinks, I still arrive at a page telling me where it's pointing versus going to the file itself. For example, this link https://github.com/ratgdo/esphome-ratgdo/blob/main/v25board_esp32_d1_mini.yaml still gets you to a file that contains static/v25board_esp32_d1_mini.yaml. Sounds like it might work for dashboard_import, but something like a link from ESPHome docs wouldn't behave as desired (unless I'm missing something).

I don't think I see a downside to storing the main file at the top. If I find a downside later, I can always move the file down and add a symlink.

@rohankapoorcom
Copy link
Contributor Author

Sounds good. Are you going to make that change? Or do you want me to?

@stephenpapierski
Copy link
Contributor

I can put in the leg work. Thanks for all the effort to simplify this!

@stephenpapierski stephenpapierski merged commit f8a0bd2 into ElevatedSensors:main Jul 26, 2024
@rohankapoorcom rohankapoorcom deleted the use-packages branch July 26, 2024 00:37
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.

2 participants