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

Create v1.0 tag #16

Closed
bogosj opened this issue Feb 24, 2021 · 19 comments
Closed

Create v1.0 tag #16

bogosj opened this issue Feb 24, 2021 · 19 comments
Assignees
Milestone

Comments

@bogosj
Copy link
Owner

bogosj commented Feb 24, 2021

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

@bogosj bogosj self-assigned this Feb 24, 2021
@bogosj
Copy link
Owner Author

bogosj commented Feb 24, 2021

We should document how to acquire the token file first.

@michaelharo
Copy link
Collaborator

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

@bogosj
Copy link
Owner Author

bogosj commented Feb 24, 2021

Fine with me.

@andig

This comment has been minimized.

@bogosj

This comment has been minimized.

@andig
Copy link
Collaborator

andig commented Feb 26, 2021

Some thoughts on the 1.0 release:

  • Should the error return for NewClient be removed before 1.0? It can never error and the method signature has changed anyway from jsgoecke.
  • I would also suggest to expose the entire oauth2.Config for sake of other users instead of just the Endpoint?
  • The go.mod targets go 1.16 which probably isn't necessary if though we might as well replace ioutil.ReadAll with io.ReadAll and the other ioutil functions with their os replacements.

@bogosj
Copy link
Owner Author

bogosj commented Feb 26, 2021

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
Copy link
Owner Author

bogosj commented Feb 26, 2021

@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
Copy link
Collaborator

andig commented Feb 26, 2021

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
Copy link
Collaborator

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

@bogosj
Copy link
Owner Author

bogosj commented Feb 26, 2021

#24 as expected, testing <1.16 is broken. I'll remove them from the matrix.

@bogosj
Copy link
Owner Author

bogosj commented Feb 26, 2021

Discussion offline with @michaelharo:

  • I'm going to rollback and continue supporting Go 1.13+. He's currently on 1.15 and the changes that are 1.16 only will break him. We can investigate removing ioutil in the future.
  • As for the (*Client, error) return type, we're going to keep that for future compatibility in the case that we offer optional args that could cause the client creation to fail.

@michaelharo
Copy link
Collaborator

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

@uhthomas
Copy link
Collaborator

uhthomas commented Mar 1, 2021

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
Copy link
Owner Author

bogosj commented Mar 1, 2021

@michaelharo @uhthomas something like #33?

@michaelharo
Copy link
Collaborator

Yeah, that's what I was thinking.

@andig
Copy link
Collaborator

andig commented Mar 25, 2021

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

@andig andig added this to the 1.0 milestone Mar 26, 2021
@michaelharo
Copy link
Collaborator

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
Copy link
Owner Author

bogosj commented May 2, 2021

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

@bogosj bogosj closed this as completed May 2, 2021
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

No branches or pull requests

4 participants