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

feat: add support for HTTP Proxy #454

Merged
merged 2 commits into from
Feb 25, 2024

Conversation

thierryvt
Copy link
Contributor

@thierryvt thierryvt commented Feb 20, 2024

defaults to using the vehicle VIN by checking (and overwriting) the vehicle_id path param and without changing any of the function signatures

  • api handler can accept either car_id or vin
  • will automatically determine which was passed in and correct to vin if necessary
  • if this fails it will attempt to just send the command as-is

@thierryvt thierryvt marked this pull request as draft February 20, 2024 22:02
@thierryvt thierryvt changed the title feat: Custom url no sig changes feat: add support for HTTP Proxy - no signature changes Feb 20, 2024
@thierryvt
Copy link
Contributor Author

@alandtse @llamafilm have a look when you've got a minute. This approach avoids changes to any of the functions or their signatures as it lays the responsibility to ensure the VIN is used inside the function that handles the api communication itself.

I've set it to draft as I still need to verify this actually works, all the tests pass but consider it a proof of concept until then.

@alandtse
Copy link
Collaborator

Please address the lint issues.

@thierryvt
Copy link
Contributor Author

done, still trying to verify it's working though. My pc isn't co-operating today...

@thierryvt thierryvt marked this pull request as ready for review February 24, 2024 19:36
@thierryvt
Copy link
Contributor Author

thierryvt commented Feb 24, 2024

@alandtse needed a minor fix but verified this works for my car, so as long as the Tesla guy wasn't lying about the old API supporting VINs this should work for everyone with 0 impact.

@llamafilm
Copy link
Contributor

I like this approach, preserving compatibility. I tested this and it works with my car. I suggest a few changes:

  • squash all these commits into one
  • Bump version to 3.10 in CHANGELOG.md, pyproject.toml, and version.py
  • Remove the "Tesla has no official API" warning in README.md

One thing to note: Reinstalling a new version of the add-on (https://github.com/thierryvt/tesla) which depends on this updated library did not actually update the library. I had to login to the homeassistant Docker container and manually reinstall the newer teslajsonpy. Hopefully that's just something weird about my dev environment and it will not affect other users...

@alandtse
Copy link
Collaborator

Do not bump versions. You do not control that. It is controlled by conventional commits.

@thierryvt
Copy link
Contributor Author

I'll squash the commits and remove the warnings.

I had the same experience with trying to test this code btw, it should work fine when this gets released and the version bumped in the tesla plugin.

- Added optional parameters to send commands via tesla-http-proxy (required for new API)
- All communication with Tesla's API now attempts to use the VIN instead of the car_id
- removed warning that Tesla has no official API support.
@thierryvt
Copy link
Contributor Author

bit messy because pycharm has a different idea of what squashing means than I do. Nothing a bit of force pushing and head resetting can't solve.

Unless there's more feedback this PR is ready to merge.

@alandtse
Copy link
Collaborator

We squash on merge so you didn't need to squash anything. But ok.

@alandtse alandtse changed the title feat: add support for HTTP Proxy - no signature changes feat: add support for HTTP Proxy Feb 25, 2024
@alandtse alandtse merged commit bdb4497 into zabuldon:dev Feb 25, 2024
4 checks passed
@thierryvt thierryvt deleted the custom-url-no-sig-changes branch February 27, 2024 19:54
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.

3 participants