Add support for time.Time and any type with private fields for DeepCopy? #47
Comments
|
As you can see it tries its best. It might work, but it is super ugly. time.Time has so many private fields and no way that I can think of that deepcopy or compare can be automatically derived. Any ideas would be appreciated. |
|
I finally have a solution: deriveDeepCopy(dst *T, src *T, deepCopyTime)
func deepCopyTime(dst *time.Time, src *time.Time) {
newTime := time.Unix(0, src.UnixNano())
dst = *newTime
}Then DeepCopy can work for any type that has private fields. This technique can then be used on all other deep functions, like Equal and Compare |
awalterschulze
changed the title
awalterschulze
added
effort-low
skill-level-beginner
good first issue
and removed
effort-medium
skill-level-intermediate
labels
it is error, why? deriveDeepCopy does not have two arguments |
|
The solution is "designed", by my comment. We are looking for someone to implement it. |
I think for better DX, would be nicer to be able to feed functions like deriveDeepCopy(dst, src, deepCopyThis, deepCopyThat, deepCopySomethingElse, deepCopyEvenMore)when you need more than few of custom copy functions. |
|
I would prefer it there wasn't any state associated with goderive, but maybe this is still possible in some way to avoid this long list of parameters. We can't have a list of deepcopy types, since they are all different types: But maybe there is a better way, that doesn't introduce state? |
It does not have to be a list though, goderive could generate implementation that accepts all those functions as individual parameters. But struct is definitely nicer approach.
It already has state associated with it in form of func noderiveDeepCopyTime(dst, src *time.Time) {
if src.IsZero() { return }
*dst = time.Unix(0, src.UnixNano()).In(src.Location())
}goderive will use it in generated code for copying time. Possibly, we're not on the same page on "state" definition in this context 🙂 |
|
Oh I see, so the state wouldn't be passed in by the user, this would just be internal for the standard library types? So in this case I still wonder how many types we would have to support, but at least it would be more easily extendible/maintainable. |
|
Oh wait, sorry. I see. So we would detect these functions also based on prefix. Hmmm, I haven't thought about that. So then where would these functions live and how would we distinguish between two functions in different user libraries? |
|
AFAIK, goderive does not work across packages, and generated functions are unexported, so there is no need for any distinction between functions in different libraries. "noderive" functions can be defined anywhere in the user code (within current package), will be picked up by goderive just like Then there also can be an option (CLI flag) for importing custom "noderive" (or rather "Noderive") functions from separate packages, so you don't have to copy-paste "noderive" functions for common types like |
|
Okay so then these noderive functions would be redefined in every package where we want to use them? Although it does seem elegant, I am a bit worried that users will find this logic hard to understand. |
From my previous comment:
For example, say there is this package ("github.com/awalterschulze/goderive/time"): package time
import "time"
func DeepCopyTime(dst, src *time.Time) {
if src.IsZero() { return }
*dst = time.Unix(0, src.UnixNano()).In(src.Location())
}Then This way we can have libraries of reusable "noderive" functions for specific types.
I think it's a matter of documentation ("Code Generation for Functional Programming" should already ring some bells though 🙂). We can also put some notice into the comment in generated code like "generated code may be using functions defined by user, see more at https://...", so people unfamiliar with goderive (or this feature of goderive) don't think someone just edited generated file. |
|
Sorry for the long wait, been pretty busy. I would prefer to not have the import as part of a command line flag, I would prefer to import it in the code as this is more explicit to the reader. This is also why I prefer the previously discussed passed in function as a parameter or the struct if we foresee to have a lot of parameters, it is all very explicit to the reader of the code. I also really like your solution, but though and I had to give it quite a bit of thought, but I think what doesn't sit well is the sacrifice of the explicitness to the reader, that I just couldn't get passed in the end, really sorry. |
|
I see your point, and looking forward to optimal solution as well. I like when things are explicit. The problem with your original approach is that we're talking about very simple things - nobody will put any custom logic into Not having to pass imports as part of command line flag would be great. What about:
package example
import "github.com/foo/bar/util"
func init() {
deriveCustom(
util.DeepCopyFoo,
util.DeepCopyBar,
)
}Then goderive will generate (in package example
import "github.com/foo/bar/util"
var (
customDeepCopyFoo func(dst, src *util.Foo)
customDeepCopyBar func(dst, src *util.Bar)
)
func deriveCustom(a func(dst, src *util.Foo), b func(dst, src *util.Bar)) {
customDeepCopyFoo = a
customDeepCopyBar = b
}
// and then use those functions in the rest of generated code...If any function is unused, |
|
Sorry for the delay, I have been a bit under the weather, will get back to you, when I am feeling a bit better. |
We have tests for gogenerate, I think it is already working But it will still require a command line argument.
Sorry, but I really dislike register functions, have been bitten by them many times and has also made it really hard to explain and debug.
See below for a spin on this idea Thinking againI was thinking quite a bit about what you trying to accomplish. Okay, so this is pretty close to your number 1 idea and with some flavor of number 3 and what I stated in the above paragraph. Instead of needing to provide custom functions for each "deep" code generation (Equal, DeepCopy, Compare), how about we provide custom types. type DeriveTime time.Time
func (this DeriveTime) Equal(that DeriveTime) bool {
return time.Time(this).UnixNano() == time.Time(that).UnixNano()
}
func (this *DeriveTime) DeepCopy(that *DeriveTime) {
}We can then provide a default library where at least time.Time is supported and more types can be contributed, but we can also provide a command line flag where users can point to their own package(s) that implement such types. Goderive would then when it encounters a type, check that it doesn't already support the custom type before attempting to generate the code. If a custom type is supported, the generated code could cast the type and call the method. What do you think? Sorry for the late reply, being sick and catching up on things is quite a lot of work. |
|
LGTM! |
|
Sounds good Could you then also write: type deriveTime foo.DeriveTimeto avoid a command line flag? |
|
Yep 👍 |
|
@awalterschulze kindly assign this to me. |
|
Haven gone through the conversation, I think I have an idea of what the problem is but I don't totally grasp the proposed solution (partly because I'm not an adept user of goderive). Problem: Absence of support for time.Time types in the deepCopy plugin Proposed Solutions:
The Agreed Solution: Finally:
|
|
@awalterschulze waiting..... |
There are other types, not just time.Time for which this solution would also need to work, given a struct with time.Time and some other std lib type as fields, it could happen that you need to pass in multiple custom functions.
Passing everything to a function that it uses, helps to keep the function pure (it contains all its own state), instead of referring to some register pattern, where there is other state outside of the function (which functions are registered, etc). I want to try and keep things as pure as possible. And preferably at least avoid register patterns.
Yeah that is fair, I see this wasn't covered very well. We allow the user to write their own custom overrides for methods, for example here DeriveTime "overrides" time.Time methods, which technically don't exist, but which goderive would check for. Now the user passes in a command line argument to goderive pointing to the package, where time.Time is provided with these extra methods. Now goderive checks this package and sees that it should prefer to call these methods, instead of looking for the method on the type or attempting to generating it, itself. The generated code should then also import this package. We can then also provide a standard library of these extra methods, once the command line parameter version is working. I hope this helps sorry for late reply, I have been very busy. |
|
@awalterschulze thanks for the detailed explanation. I'd keep my questions shorter next time, thanks. |
|
@awalterschulze I've also been a bit busy myself. Just had the time to read through your comment. Seems elegant, so we allow the user specify how each field (dst & src) should be compared and copied. In addition, we can provide a list of std libraries containing our own extra methods so that in case a user doesn't specify a custom method, goderive konws to use one of the std libraries we provie. |
|
@awalterschulze could you point me to the right section of the codebase where "deepcopy" code generation takes place. I'm trying to understand how goderive walks through the users code to find unimplemented method calls with the appropriate "prefixes" and then generates the desired code. |
Yes each field type, can be overridden with a new way to copy.
Yes :)
Yes here is the deepcopy codegen plugin
The Now the deepcopy plugin would also need to know about other types it needs to check for methods, types that the current type can be cast to, which is one of the big parts that is not done. |
|
I'm back 😎😎, fell ill a couple of times since my last comment and had to catch up with work after regaining my health. I'd be starting off where I stopped. Hope to still get enough support from @awalterschulze |
|
There's a huge learning curve for me here, it's my first time working on a code generator. I've spent the last few days reading the comments and the goderive itself. I have some understanding of how DeepCopy works but I'm not quite sure where to start implementing this feature. Any resource to help bring me up to speed would be appreciated. |
|
I am sorry, I never had time to write documentation about how to even implement a plugin, which is my bad. I should have done that when I was developing goderive, because now I definitely don't have time :( Currently the only way is to look at the plugins that have already been implemented and hope to find something similar to what you want to do. I am sorry this is not good, but it is all we have at the moment. Some advice though:
|
|
Thanks @awalterschulze, writing the tests seems a bit tricky given that I'd be checking the generated code as opposed to testing say a built in type of golang. I'd checkout the tests you wrote again to understand it better and do a TDD, hopefully that helps better. Thanks. |
|
I think I'd pause on this, it's a bit difficult to put in the time whilst working. @awalterschulze maybe I should start with a simpler task that wouldn't require me studying much (just to get myself started) before coming back to this. |
awalterschulze
added
skill-level-advanced
effort-high
and removed
effort-low
skill-level-beginner
good first issue
labels
|
Yeah that is fair, I think as we have progressed on a solution, this issue has become mislabeled. It isn't an easy issue or a good one to start with. This is my bad for forgetting to update the labels. I can see now a lot of my issues are labeled. I am sorry. What you can try is create a new Recursive use case, since this won't be replaced with generics. For example: Marshal (protobuf), Unmarshal (protobuf), VerboseEqual (it returns an error or diff), CopyTo (that takes two different types with some similar field names and types and returns an error at generation time, if it couldn't copy over all fields from src to dst), maybe you have a better idea? |
Like the title reads. When generating a struct containing a time.Time field I get this use of un-exported, e.g.
time.zone:The text was updated successfully, but these errors were encountered: