feat: deprecate Timestamp and TimestampTZ visit functions #273
Conversation
mbwhite
force-pushed
the
fix-deprecated-typevistor
branch
from
3bb8765
to
7912220
Compare
| @@ -23,9 +23,17 @@ public interface TypeVisitor<R, E extends Throwable> { | |||
|
|
|||
| R visit(Type.Time type) throws E; | |||
|
|
|||
| R visit(Type.TimestampTZ type) throws E; | |||
| @Deprecated | |||
| default R visit(Type.TimestampTZ type) throws E { | |||
There was a problem hiding this comment.
Marking the visitor methods as deprecated makes sense to me, but I'm not sure if providing a default implementation is what we want here. There is TypeThrowsVisitor to allow users to implement partial visitors, but ideally the core visitor should force users to be exhaustive.
There was a problem hiding this comment.
thanks @vbarua
I would agree that the TypeThrowsVisitor is useful for partial visitor implementations; and that the core visitor should be exhaustive. My thinking is that it's not appropriate or helpful to ask implementations to implement a deprecated method. (it's a relatively new codebase as well not at v1 as well).. A default implementation could even defer to the new API if there was a 1:1 mapping between old and new APIs (which in this case I think there is).
Marking as @deprecated certainly; will go with your thoughts on the default implementation.
There was a problem hiding this comment.
My thinking is that it's not appropriate or helpful to ask implementations to implement a deprecated method.
I suspect that those 2 fields will be around for a while (order of months to years) as various other systems will need updates to switch over. I think it's safest to have users of the library keep that in mind by forcing them to handle it. That being said
default implementation could even defer to the new API if there was a 1:1 mapping between old and new APIs
if we can manage this that would solve this issue for me.
There was a problem hiding this comment.
Had a look at being going from the Timestamp to the PrecisionTimestamp variants. The PrecisionTimestamps don't have a default value for precision - so unless a value is available to create that when a default implementation won't work.
@vbarua would you rather modifying this pr to just do @depecated additions, or close this one and open a fresh one?
There was a problem hiding this comment.
Let's start with just deprecating for now.
There was a problem hiding this comment.
@vbarua I've updated the PR to just include the deprecation annotation
By marking the function as @deprecated means that users will know the method is deprecated and not get warnings specifically about their code. An implementation will still be required though. Signed-off-by: MBWhite <[email protected]>
mbwhite
force-pushed
the
fix-deprecated-typevistor
branch
from
7912220
to
c1d53cd
Compare
mbwhite
changed the title
vbarua
changed the title
By marking the function as @deprecated and providing a default implementation makes users code a lot cleaner.
Directly implementations of the interface previously would need to implement these methods and cope with the Deprecation warning.
With this change if they are using it, no problems; but they are not obliged to implement it.