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

Fixed up concerns about backwards compatibility #39

Closed
wants to merge 3 commits into from
Closed

Fixed up concerns about backwards compatibility #39

wants to merge 3 commits into from

Conversation

pjebs
Copy link

@pjebs pjebs commented Apr 4, 2017

@cenkalti

  • Changed PermanentError to FatalError (name is self-explanatory)
  • Added support for Go1.7+'s context package whilst providing support for context for older versions of Go.

PJ added 3 commits April 4, 2017 09:44
FatalError is a better name because it is self-explanatory.
- Your travis settings are set to go1.3.3 so I didn’t change the unit
tests.
@coveralls
Copy link

coveralls commented Apr 4, 2017

Coverage Status

Coverage decreased (-4.6%) to 77.723% when pulling 422bf8a on skylovely:master into 5d150e7 on cenkalti:master.

@cenkalti
Copy link
Owner

cenkalti commented Apr 4, 2017

@pjebs

  • You are introducing a new error type FatalError that is copy/paste of PermanentError. This creates confusion and does not help in any way. The change is unnecessary.

  • You also break builds by changing import path of context package for Go users using version >=1.7. Read the thread here for details: Go 1.7 uses import "context" grpc/grpc-go#711

Please open an issue to discuss before sending more pull requests.

@cenkalti cenkalti closed this Apr 4, 2017
@pjebs
Copy link
Author

pjebs commented Apr 4, 2017

@cenkalti
I have a build tag at the top for the different context type. +// +build go1.7
In terms of breaking builds, it won't break builds for go less than 1.7.
For builds of go1.7+, it will potentially break builds, but that is because it was a mistake in the first place to NOT have the build tags. Those go1.7+ uses will probably WANT to use the context package directly from the standard library.
What is your transition plan as Go goes from 1.7 -> 1.8 -> 1.9. Are you going to ignore the standard library context package until the year 2039?

@pjebs
Copy link
Author

pjebs commented Apr 4, 2017

Regarding the addition of FatalError. It is done because the name itself is self-explanatory. So while the change is technically unnecessary, it is backwards compatible AND it improves your package overall.

@cenkalti
Copy link
Owner

cenkalti commented Apr 4, 2017

  • It is not a mistake to not using build tags. Source code (thus import path of context package) should not depend on the version of the compiler used. Every package that imports context package has the same problem. You have duplicated the contents of the context.go in a different file, it is ugly. If type aliases come with 1.9, then the transition will be seamless: Revert "Update to standard library context package." golang/protobuf#326 (comment)

  • Maybe the name PermanentError is not perfect but it seems okay to me. It is decided once and I don't want to change it. If it is not self-explanatory we can explain better in docs.

@cenkalti cenkalti mentioned this pull request May 10, 2017
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.

None yet

3 participants