awalterschulze / goderive

Derives and generates mundane golang functions that you do not want to maintain yourself
Apache License 2.0
1.24k stars 43 forks source link

generate doc comments #27

Closed rogpeppe closed 7 years ago

rogpeppe commented 7 years ago

It would be nice to generate doc comments for the generated functions, so that even someone with no knowledge of goderive can immediately see what the contract of the functions is.

awalterschulze commented 7 years ago

These functions will typically be private functions, so its not necessary for go vet or go lint. Well except if someone is changing the prefix to a capital letter.

But I am still totally open to this idea.

rogpeppe commented 7 years ago

It might not be necessary for go vet or go lint, but I still think it's a good idea to generate doc comments, so that it's clear what they're doing - if someone jumps to the definition of the generated function, they'll see quickly what it's supposed to be doing.

awalterschulze commented 7 years ago

This can be added on a per plugin basis.
So we can slowly move to this standard, without any refactoring. What would be an example comment?

rogpeppe commented 7 years ago

something like:

// deriveAllInt reports whether predicate returns true for all the given elements
// in the given slice. 
func deriveAllInt(predicate func(int) bool, slice []int) bool {

// copyToX recursively copies the contents of src into dst.
func copyToX(dst, src *X) {

That kind of thing.

awalterschulze commented 7 years ago

Ok great so no type specific information in the comment, but it still contains the specific function name and gives an appropriate description. That should not be too hard :)

awalterschulze commented 7 years ago

I have added those two as a start https://github.com/awalterschulze/goderive/commit/0eca5e956fd4dc78d947a32e6e45fa4263d57dd4

rogpeppe commented 7 years ago

Thanks for doing that. It would be nice if the parameter names matched the parameter names mentioned in the doc comment though. (src,dst vs this,that)

awalterschulze commented 7 years ago

Ah yes obviously, my bad. Fixed https://github.com/awalterschulze/goderive/commit/6fa103e6f18ad6e0e81684340008063b49886987

rogpeppe commented 7 years ago

Interestingly, that's now made the order of the arguments clearer to me. I wonder whether the CopyTo primitive arguments should be in the other order, by analogy to io.Copy, reflect.Copy, hex.Encode etc.

awalterschulze commented 7 years ago

I also wonder, because when you have a method, the current order of CopyTo is quite clear.

func (this *A) CopyTo(that *B) {
   deriveCopyTo(this, that)
}

But when its like all the other libs then its unintuitive

func (this *A) CopyTo(that *B) {
   deriveCopyTo(that, this)
}

But there is also something said for following a standard. So I really don't know.

rogpeppe commented 7 years ago

I haven't seen many implementation of CopyTo that look like that. Equally valid would be:

func (a *A) Set(b *A) {
    deriveCopy(a, b)
}

So perhaps the plugin could be named "copy" rather than "copyto". I think the correspondence with reflect.Copy is strong.

awalterschulze commented 7 years ago

I would like to avoid having plugin names that conflict with builtin function names. Because I think there is a next level to this project (maybe just a flag) where each function name simply becomes the plugin name.

rogpeppe commented 7 years ago

in which case then copyValue might work. tbh though, I think I'd always want some kind of prefix to the function names, unless I explicitly rename. And almost always, I'd want the name to have some kind of suffix that's specific to the type anyway, so I don't think there's much danger of a clash with the builtin copy primitive.

awalterschulze commented 7 years ago

I agree and I won't take this prefix away for goderive, don't worry. I really like the explicitness that makes it clear that this is a generated/derived function.

Despite a project that I am planning that will need to use goderive as a library. There is also the fact that the package name and plugin name will then be different, because of reserved words. So I would still like to avoid using builtin function names.

copyValue sounds like the input parameters are not pointers. But I am still open to a better name. I just can't think of one.

awalterschulze commented 7 years ago

But please keep trying.

rogpeppe commented 7 years ago

There is also the fact that the package name and plugin name will then be different, because of reserved words.

"copy" isn't a reserved word, and you can always use a different import name if you like. (you could even have a convention that plugins register themselves in a central repository at init time if you like, at which point you'd only need to "import _" all the plugins).

I still think that "copy" is the best name for it, even if you don't use that name internally for whatever reason.

rogpeppe commented 7 years ago

Actually, what about "deepcopy"? That's essentially what it's doing, right?

awalterschulze commented 7 years ago

Yes I like that. I'll change it to deepcopy :)

awalterschulze commented 7 years ago

Done https://github.com/awalterschulze/goderive/commit/0dcfd1c503c7a2aa120edb2807404a4d10962e20

awalterschulze commented 7 years ago

All functions now have comments, obviously I am still open to corrections and suggestions for better descriptions.