You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I think this changed as part of the zap/zapcore split, but the original plan with fields was to keep the implementation details completely private, and the only way to use a field would be to AddTo(Encoder).
It seems like FieldType is exported only to let the zap package expose field constructors, but I think what we should unexport all implementation details and instead:
Expose the same field constructors in the zapcore package
The zap package just wraps the underlying zapcore functions, and doesn't need access to the internals of a field.
This gives us more flexibility to change Field implementation details in future.
It's a little late for 1.0, but perhaps in a far away 2.0.
I'll add this to a wishlist issue for a future major version release; we can discuss details then.
For the 1.0, I was weighing this implementation detail leak against the discoverability/documentation confusion cost of duplicating the field constructors. If we're going to keep fields completely opaque, we should just collapse everything back into one package - there are waaaaay too many field constructors (even for just the basic types) to make duplicating them worthwhile.
I think this changed as part of the
zap
/zapcore
split, but the original plan with fields was to keep the implementation details completely private, and the only way to use a field would be toAddTo(Encoder)
.It seems like
FieldType
is exported only to let thezap
package expose field constructors, but I think what we should unexport all implementation details and instead:zapcore
packagezap
package just wraps the underlyingzapcore
functions, and doesn't need access to the internals of a field.This gives us more flexibility to change
Field
implementation details in future.It's a little late for 1.0, but perhaps in a far away 2.0.
Thoughts? @akshayjshah @billf
The text was updated successfully, but these errors were encountered: