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.

Sign up for GitHub

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

Jump to bottom

Fix missing field error for charging event #359

Merged
merged 5 commits into from
Nov 30, 2024
Merged

Fix missing field error for charging event #359

merged 5 commits into from
Nov 30, 2024

Conversation

fursov
Copy link
Contributor

@fursov fursov commented Nov 27, 2024

This is draft change for #356.

@fursov
Copy link
Contributor Author

fursov commented Nov 27, 2024

Seems this works for the event charging-status-changed. My Superb does not fire change-soc event, would be nice of this is also tested against that event.

@WebSpider WebSpider marked this pull request as draft November 27, 2024 18:07
@WebSpider WebSpider added the bugfix This PR contains a bugfix label Nov 27, 2024
@fursov
Copy link
Contributor Author

fursov commented Nov 27, 2024

@WebSpider there are two options how this fix can be done:

  • one is to use isinstance to check which class fires the event (done in the current change)
  • two is to use event name to distinguish the data structure
    What would you prefer? I slightly incline towards event name, but then the event class is not visible here anymore, we would just implicitly assume the fields in the data...

WebSpider
WebSpider requested changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@WebSpider WebSpider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

Let's try and keep the classes in place unless it really complicates things. Using classes makes it really clear what type of messages we are passing around.

custom_components/myskoda/coordinator.py Outdated Show resolved Hide resolved
@fursov fursov requested a review from WebSpider November 29, 2024 09:03
@fursov fursov marked this pull request as ready for review November 29, 2024 09:04
@fursov
Copy link
Contributor Author

fursov commented Nov 29, 2024

As note: this PR only to fix the error report. I created improvement ticket #370 to refactor the function properly.

@prvakt prvakt mentioned this pull request Nov 29, 2024
Merged
WebSpider
WebSpider reviewed Nov 29, 2024
View reviewed changes
Copy link
Contributor

@WebSpider WebSpider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a nice bugfix that lays some good groundwork!
One question left, put inline.

custom_components/myskoda/coordinator.py Show resolved Hide resolved
custom_components/myskoda/coordinator.py Show resolved Hide resolved
@prvakt
Copy link
Collaborator

prvakt commented Nov 30, 2024

tested with my Superb and looks OK.. also #368 work correctly when I used this change ;)

WebSpider
WebSpider approved these changes Nov 30, 2024
View reviewed changes
Copy link
Contributor

@WebSpider WebSpider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@WebSpider WebSpider merged commit 8f0f639 into skodaconnect:main Nov 30, 2024
3 checks passed
@prvakt prvakt mentioned this pull request Nov 30, 2024
Closed
@fursov fursov deleted the fix-charging-event branch December 1, 2024 08:09
fursov added a commit to fursov/homeassistant-myskoda that referenced this pull request Dec 7, 2024
@fursov
Update LastUpdated sensor value on service events
3c76c64 
As discussed in skodaconnect#359,
the car_captured_timestamp can be updated with timestamp value
available from service events.
@fursov fursov mentioned this pull request Dec 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WebSpider WebSpider WebSpider approved these changes

Assignees
No one assigned
Labels
bugfix This PR contains a bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fursov @prvakt @WebSpider