search

bogosj / tesla

Provides a wrapper around the API to easily query and command a Telsa car.
Other
23 stars 18 forks source link

Use more descriptive struct field names #29

Closed michaelharo closed 3 years ago

bogosj commented 3 years ago

@uhthomas and @andig might have some comments on this. Unclear if their services use these values. In general I'm in favor of the change, but I'm not clear on what deviating from the JSON fields would do to existing consumers.

uhthomas commented 3 years ago

I'm a fan of making the API a little nicer. "FrontTrunk" is much clearer than "Ft", but I am wary of changing the interface{} values. The Tesla JSON API uses a lot of null and knowing the difference between unset, zero and actual will become challenging otherwise. Don't get me wrong, using an interface{} is not correct and it should be changed.

michaelharo commented 3 years ago

I'm a fan of making the API a little nicer. "FrontTrunk" is much clearer than "Ft", but I am wary of changing the interface{} values. The Tesla JSON API uses a lot of null and knowing the difference between unset, zero and actual will become challenging otherwise. Don't get me wrong, using an interface{} is not correct and it should be changed.

I haven't checked if this works, but would making the fields pointers resolve the concern about null vs zero value?

andig commented 3 years ago

+1 for the naming changes. Compatibility is broken anyway, so why not improve usability as well?

bogosj commented 3 years ago

Is there anything left to do here before merging?

This plus the login PR should make #16 doable?

andig commented 3 years ago

+1

michaelharo commented 3 years ago

I'm unclear on the direction to take here. I'd prefer to use nicer types and nicer names. Should I flip every field over to a pointer so it's possible to check unset vs zero value?

bogosj commented 3 years ago

Suggestion: we defer these decisions for now and tag v1.0 as close to compatibility with jsgoecke as possible (plus the auth bits). We can then figure out what cleanup breaking changes should go into v1.1.

michaelharo commented 3 years ago

How about we split this change into 2 parts... The first part, renaming some fields, goes into 1.0. We can then defer the decision about types and zero vs nil.

bogosj commented 3 years ago

I think this is the last thing we are waiting on to tag v1.0?

andig commented 3 years ago

I think this is the last thing we are waiting on to tag v1.0?

We need #43 too :O.

How about we split this change into 2 parts... The first part, renaming some fields, goes into 1.0. We can then defer the decision about types and zero vs nil.

Looking at this again, I would suggest to change all field names/types before 1.0 and leave the nil discussion until later. These changes would be easy to pick ip without need to change the logic.

michaelharo commented 3 years ago

sorry about the delay. The time type change has been reverted.

andig commented 3 years ago

Lgtm

bogosj commented 3 years ago

LGTM. Merge as you see fit.