search

bogosj / tesla

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

Create v1.0 tag #16

Closed bogosj closed 3 years ago

bogosj commented 3 years ago

I'd like to consider a v1.0 tag for the library after #8 is closed, preferable after #15 is merged.

bogosj commented 3 years ago

We should document how to acquire the token file first.

michaelharo commented 3 years ago

If I have time I'd like to improve the tests and possibly change some data types in the structs before v1.0.

bogosj commented 3 years ago

Fine with me.

andig commented 3 years ago

Could we expose the login stuff as api? My use is integrated with another application, running a separate cmd is not an option. I've basically done the same as here- copied and integrated- but I feel that it really deserves its own auth api?

bogosj commented 3 years ago

@andig going to track that here: https://github.com/bogosj/tesla/issues/22. I don't want to gate tagging v1.0 of a usable API wrapper on that. Hiding both of our comments here.

I've just finished up linting cmd/..., would you be interested in refactoring out the parts you care about to /login and having /cmd/login depend on it?

andig commented 3 years ago

Some thoughts on the 1.0 release:

bogosj commented 3 years ago

re: oauth2.Config - going to leave that to @michaelharo to think about as he made those changes.

re: go.mod 1.16 - is the suggestion we drop that directive completely? I'll swap out the usage of ioutil in another PR.

bogosj commented 3 years ago

@andig I'm using ioutil.ReadFile which is replaced by os.ReadFile doesn't exist <1.16. I'm not clear on the convention of how fast it's expected people would update to 1.16. I'm OK declaring that this library only works in 1.16+ but I'll wait to hear from you and @michaelharo and @uhthomas before I swap out the usages of ioutil.

andig commented 3 years ago

As long as 1.16 is targeted we can swap the functions. I don‘t think theres any need to require it, could probably be 1.13. I wouldn‘t mind 1.16 as I love the new features even if we don‘t need them here.

uhthomas commented 3 years ago

I think 1.16 is reasonable. The Go compatibility promise makes this quite safe.

bogosj commented 3 years ago

https://github.com/bogosj/tesla/pull/24 as expected, testing <1.16 is broken. I'll remove them from the matrix.

bogosj commented 3 years ago

Discussion offline with @michaelharo:

michaelharo commented 3 years ago

Should BaseURL and StreamingURL be exported? I'm wondering if they should be part of the 1.0 API.

uhthomas commented 3 years ago

Maybe not exported as a field, but definitely should be settable via functional options. It's essential for black box unit testing, and integration tests.

bogosj commented 3 years ago

@michaelharo @uhthomas something like https://github.com/bogosj/tesla/pull/33?

michaelharo commented 3 years ago

Yeah, that's what I was thinking.

andig commented 3 years ago

I will send 2 more small PRs. One rename, one handling token expiry. The latter should go in before 1.0:

michaelharo commented 3 years ago

I just noticed that my change to make struct member names more meaningful is potentially inconsistent. Not sure if we should care...

It uses both DriverFrontDoor and FrontDriverWindow. This was based on the order of the letters in the json struct, but perhaps it Front/Back or Driver/Passenger should always be ordered the same in the names without regard to if it's a Door or Window?

bogosj commented 3 years ago

I'm ambivalent to the inconsistency - the inconsistency matches the structure presented by the API. I'm going to tag HEAD as v1.0.