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

Revert pure virtual functions in UART component from #5920 #5932

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

DrCoolzic
Copy link
Contributor

@DrCoolzic DrCoolzic commented Dec 14, 2023

What does this implement/fix?

PR5920 has introduced a braking change by adding two PURE virtual functions. Therefore ANY class derived from UARTComponent will not compile anymore !!! Big badaboom

This fix keeps the two new virtual functions introduced in PR5920 but not as PURE VIRTUAL functions.
What's amazing is that there was absolutely no reason to make them pure virtual ???

For more info see my comment on #5920

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)
  • Other

Related issue or feature (if applicable): fixes
#5920

Test Environment

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

Example entry for config.yaml:

NA

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:

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (uart) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@SeByDocKy
Copy link
Contributor

Rework fine now with this PR thx :)

@DrCoolzic DrCoolzic changed the title Drcoolzic fix pr5920 breaking change Drcoolzic fixed PR5920 breaking changes Dec 14, 2023
@edwardtfn
Copy link
Contributor

Thanks for your prompt response to this, @DrCoolzic.

@kbx81 kbx81 added this to the 2023.12.0b3 milestone Dec 15, 2023
@kbx81 kbx81 changed the title Drcoolzic fixed PR5920 breaking changes Revert pure virtual functions in UART component from #5920 Dec 16, 2023
@DrCoolzic
Copy link
Contributor Author

A big thank to @clyde @ssieb for helping me fix the commit history (went done from 148 to 1 :) )

@DrCoolzic
Copy link
Contributor Author

DrCoolzic commented Dec 16, 2023

Sorry for using incorrect terminology. It seems like breaking change has a specific meaning in esphome
see https://discord.com/channels/429907082951524364/429907082955718657/1185320271721271387 :

Apprently C++ breaking changes to a component API (e.g. https://www.cppdepend.com/detect-api-breaking-changes) is not considered as a breaking change for esphome. As I understand it, this term only applies if the yaml configuration is to be changed.

The interface change breaks the component I submitted 6 months ago, but as it hasn't been reviewed yet, it's still considered an external component.

jesserockz
jesserockz previously approved these changes Dec 17, 2023
@jesserockz jesserockz mentioned this pull request Dec 17, 2023
13 tasks
@jesserockz jesserockz enabled auto-merge (squash) December 17, 2023 23:42
@jesserockz jesserockz merged commit 1d37edb into esphome:dev Dec 18, 2023
48 checks passed
jesserockz added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com>
@jesserockz jesserockz mentioned this pull request Dec 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2023
@DrCoolzic DrCoolzic deleted the drcoolzic_fix_pr5920 branch January 30, 2024 17:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants