Skip to content

Conversation

@krzysdabro
Copy link
Contributor

@krzysdabro krzysdabro commented Oct 6, 2024

Proposed change

This PR adds integration for Volkswagen WeConnect service using WeConnect-python library.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

@home-assistant home-assistant bot marked this pull request as draft October 6, 2024 16:17
@home-assistant
Copy link

home-assistant bot commented Oct 6, 2024

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.

Copy link
Contributor

@autinerd autinerd left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution! Here are some first comments.

EDIT: Can you please add a link to the library in the PR description?

@krzysdabro
Copy link
Contributor Author

@autinerd Thank you for the review. I have fix the issues and included link to library in description.

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

This library is scraping the web and that is not allowed via ADR-004. The exception states that every field should be queried at the same time, thus that means we should not use regex. Have a check at https://github.com/joostlek/python-volkswagen, we use the build in HTML parser and that is allowed in Home Assistant

@home-assistant home-assistant bot marked this pull request as draft October 7, 2024 11:34
@krzysdabro
Copy link
Contributor Author

@joostlek As far as I know, this library actually uses the JSON API for the data updates, as you can see here and here. he only time it interacts with the web application is during the authentication process, which, from my understanding, is allowed under ADR-004. While I must admit your library offers a much cleaner solution for the authentication process, it doesn’t provide further data on the vehicles.

What would you suggest as the solution here?

@joostlek
Copy link
Member

I know, I am also not saying that my library is the way to go, its just that authentication like that would be accepted, but auth like in this PR would not, since it uses regex parsing

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Dec 12, 2024
@krzysdabro
Copy link
Contributor Author

FYI I have created a PR to refactor auth in the lib: tillsteinbach/WeConnect-python#223

@github-actions github-actions bot removed the stale label Dec 15, 2024
@krzysdabro krzysdabro force-pushed the weconnect branch 3 times, most recently from 3e924e6 to e02affa Compare December 25, 2024 22:18
@krzysdabro
Copy link
Contributor Author

@joostlek The authentication process in the library has been refactored to use HTML parsing. I've upgraded the package, updated the translations, and added a quality scale checklist. Could you take a look when you have some spare time?

@krzysdabro krzysdabro marked this pull request as ready for review January 13, 2025 20:44
@home-assistant home-assistant bot requested review from autinerd and joostlek January 13, 2025 20:44
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Mar 15, 2025
@github-actions github-actions bot closed this Mar 22, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2025
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.

3 participants