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

Add bmw-unit-preferences #449

Merged
merged 6 commits into from
May 29, 2022
Merged

Add bmw-unit-preferences #449

merged 6 commits into from
May 29, 2022

Conversation

rikroe
Copy link
Member

@rikroe rikroe commented May 28, 2022

Proposed change

Apparently, one has to give unit preferences when making API calls now.

It is possible to change between metric/imperial units by:

  • initializing MyBMWAccount(..., use_metric_units=False)
  • calling account.set_use_metric_units(False) after init.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to this library)
  • 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.
  • Tests have been added to verify that the new code works.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #449 (483090c) into master (69040fc) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #449   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1248      1255    +7     
=========================================
+ Hits          1248      1255    +7     
Flag Coverage Δ
3.10.x 100.00% <100.00%> (ø)
3.6 100.00% <100.00%> (ø)
3.7 100.00% <100.00%> (ø)
3.8 100.00% <100.00%> (ø)
3.9 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bimmer_connected/models.py 100.00% <ø> (ø)
bimmer_connected/account.py 100.00% <100.00%> (ø)
bimmer_connected/api/client.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/charging_profile.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/doors_windows.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/fuel_and_battery.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/location.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/remote_services.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/reports.py 100.00% <100.00%> (ø)
bimmer_connected/vehicle/vehicle.py 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69040fc...483090c. Read the comment docs.

@FlyingDiver
Copy link

Shouldn't that be a named parameter to the function so that it can be easily overridden instead of having to specify the entire header dict?

@rikroe
Copy link
Member Author

rikroe commented May 28, 2022

It could be written as a named parameter, yes. The question however would be how to control the calls, as this function is called in the backend.

I could image that it makes sense to add unit properties to MyBMWClientConfiguration as we cannot really distinguish based on the region.

If the library is used from Home Assistant, we could use use the HA settings, and if used somewhere else it is at least configurable (through passing MyBMWClientConfiguraition or a setter method on MyBMWAccount).

Any thoughts?

Also, as you're based in NA if I remember correctly - could you please check how the values behave for you? Apparently in ROW, the odometer shows the value in mi but returns the unit as km.

@FlyingDiver
Copy link

I was hoping it could be something passed through the MyBMWAccount constructor, as that's the primary entry point for the API. Or set as an attribute after creation.

I don't understand the results I get now. I get mileage in mi, but fuel volume in liters. ???

@rikroe
Copy link
Member Author

rikroe commented May 28, 2022

I was hoping it could be something passed through the MyBMWAccount constructor, as that's the primary entry point for the API. Or set as an attribute after creation.

That should be possible, will have a look tomorrow.

I don't understand the results I get now. I get mileage in mi, but fuel volume in liters. ???

I don't know what the API expects or responds in this case.
Could you try multiple values, to figure out what exactly we have to send for metric/imperial units?

@FlyingDiver
Copy link

Honestly, I'm not sure I have a way to set that value now. Unless I edit the code installed in site_packages. Unfortunately, my setup can't use a venv.

@rikroe
Copy link
Member Author

rikroe commented May 29, 2022

If your setup does not reload the package from pypizon restart, editing the files in site_packages would be a viable, albeit hacky solution.

@rikroe
Copy link
Member Author

rikroe commented May 29, 2022

Just figured out that I am able to change the unit preferences in the MyBMW app.

With a little bit of refactoring, it is now possible to either change units by

  • initializing MyBMWAccount(..., use_metric_units=False)
  • calling account.set_use_metric_units(False) after init.

@rikroe rikroe requested a review from gerard33 May 29, 2022 11:31
@sincze
Copy link

sincze commented May 29, 2022

Just figured out that I am able to change the unit preferences in the MyBMW app.

As that seems to be correct my MyBMW app is set for Liters and KM in the UNITS section, however still shows the tyre pressure in the APP in PSI instead of Bar, I think they still have some work to do at BMW.

@rikroe rikroe merged commit f1b6557 into master May 29, 2022
@github-actions
Copy link

github-actions bot commented Jun 6, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2022
@rikroe rikroe deleted the unit-preferences branch April 14, 2023 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distances in miles instead of km
5 participants