8000
Skip to content

Remove the choice for MQTT logging if it is disabled - #7723

Merged
jesserockz merged 3 commits into
esphome:devfrom
solarkennedy:mqtt_logging_disabled_choice
Nov 11, 2024
Merged

Remove the choice for MQTT logging if it is disabled#7723
jesserockz merged 3 commits into
esphome:devfrom
solarkennedy:mqtt_logging_disabled_choice

Conversation

@solarkennedy
Copy link
Copy Markdown
Contributor

This is a second pass at esphome/issues#6046

I have a situation where sometimes I have MQTT logging enabled, but sometimes not depending on the device.

This PR removes the choice so that one doesn't have to remember that MQTT logging is disabled, so that esphome logs goes right for the API method (if available).

What does this implement/fix?

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

Pull request in esphome-docs with documentation (if applicable):

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

This is a second pass at esphome/issues#6046

I have a situation where sometimes I have MQTT logging enabled, but
sometimes not depending on the device.

This PR removes the choice so that one doesn't have to remember that
MQTT logging is disabled, so that `esphome logs` goes right for the API
method (if available).
@solarkennedy
solarkennedy requested a review from a team as a code owner November 5, 2024 17:39
@probot-esphome probot-esphome Bot added core small-pr PR < 30 lines labels Nov 5, 2024
@codecov-commenter
codecov-commenter commented Nov 5, 2024
Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 16.66667% with 10 lines in your changes missing coverage. Please review.

Project coverage is 53.95%. Comparing base (4d8b5ed) to head (34cb168).
Report is 1558 commits behind head on dev.

Files with missing lines Patch % Lines
esphome/__main__.py 16.66% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #7723      +/-   ##
==========================================
+ Coverage   53.70%   53.95%   +0.24%     
==========================================
  Files          50       50              
  Lines        9408     9775     +367     
  Branches     1654     1347     -307     
==========================================
+ Hits         5053     5274     +221     
- Misses       4056     4175     +119     
- Partials      299      326      +27     

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

Comment thread esphome/__main__.py Outdated
Comment thread esphome/__main__.py
@esphome
esphome Bot commented Nov 5, 2024
Copy link
Copy Markdown
Contributor

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@esphome
esphome Bot marked this pull request as draft November 5, 2024 21:10
@solarkennedy
solarkennedy marked this pull request as ready for review November 6, 2024 04:29
@esphome
esphome Bot requested a review from jesserockz November 6, 2024 04:29
@solarkennedy
Copy link
Copy Markdown
Contributor Author

Added in requested changes

@jesserockz
jesserockz merged commit c35240c into esphome:dev Nov 11, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

core small-pr PR < 30 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting MQTT log topic to null produces error

3 participants

0