CLI: Add 'type-doc' option to copy struct doc to interface

markus-wa commented 5 years ago

Change

This PR adds a new CLI option --type-doc (-D). When this flag is set the generated interface will have the struct's documentation appended to it's own.

Due to necessity this PR enables multi-line interface-comments for -y.

Fixes #25

I'm not sure if it would be a good idea to search/replace the original struct name in the interface doc - it could have some unwanted side-effects in some cases.

Example A (with `-y`):

// MyType can do some interesting stuff.
type MyType struct {
}

ifacemaker -f mytype.go -s MyType -D -y "MyInterface is an interface for MyType, intended to be used when mockability is needed." -i MyInterface

// MyInterface is an interface for MyType, intended to be used when mockability is needed.
// MyType can do some interesting stuff.
type MyInterface struct {
}

Example B (no `-y`):

// MyType can do some interesting stuff.
type MyType struct {
}

ifacemaker -f mytype.go -s MyType --type-doc -i MyInterface

// MyInterface ...
// MyType can do some interesting stuff.
type MyInterface struct {
}

markus-wa commented 5 years ago

Sure yeah, I'll try to get to it tomorrow 😄

vburenin commented 5 years ago

Thanks. It should be pretty trivial, majority of basic test checks are already in place.

DenLilleMand commented 5 years ago

Inherently this feature is not going to adhere to golint conventions, i don't know how realistic it is to fix this kind of issue. You could assume that people write code that follows golint suggestions and then you could just replace the initial word with the interface name to fix it, but in the cases where golint is not followed it is going to look just weird.

Quick suggestion: Update the README with this change.

markus-wa commented 5 years ago

I've updated the readme and added unit + integration tests.

golint looks happy on my end, the original comment is only appended after the interface comment for this reason.

Edit: should I also add a concrete example to the README with the output? I wasn't sure and didn't want to unnecessarily clutter it and only added the --help output.

markus-wa commented 5 years ago

I noticed something when writing the integration tests: there seem to be some unintended newlines, see https://github.com/vburenin/ifacemaker/pull/26/commits/97c551f0acbf6c8544327cb378317296d6c74431#diff-7599123d5879588e5dcf8cae3d550245R184

I decided not to touch it as it doesn't relate to the PR.

DenLilleMand commented 5 years ago

I've updated the readme and added unit + integration tests.

golint looks happy on my end, the original comment is only appended after the interface comment for this reason.

Edit: should I also add a concrete example to the README with the output? I wasn't sure and didn't want to unnecessarily clutter it and only added the --help output.

arg okay, i just didn't read it properly. @markus-wa is this behavior actually wanted? in my world the functionality makes more sense if it applies as a default, so when -y is not applied. As a default it then rewrites the comment from the struct exchanging the structName in the comment with the interface name assuming people have golint valid code.

But i mean, i might be wrong, maybe this kind of behavior is OK for a auto-generated interface, after all it is hard to guess what kind of documentation would make sense given our current information :)

markus-wa commented 5 years ago

I thought about rewriting the doc replacing the original type name with the interface name. But in some cases this will lead to strange behavior.

Here's what I use it for: https://github.com/markus-wa/demoinfocs-golang/blob/interface-generator/parser_interface.go

In this case we should replace To start off you may use Parser.ParseHeader() to parse the demo header with IParser.ParseHeader() - but if we just search/replace Parser/IParser that's also gonna replace the following line // [...] is done via NewParser(). with // [...] is done via NewIParser(). which would be wrong. So we'd need to do fairly fancy punctuation checks to catch most cases, which exceeds the scope of this PR.

It gets even worse if the original type is a single word and private. Then the type name could be used as a noun in the doc, and we don't want to replace that.

E.g.

// parser ... 
// This parser does x in fashion y
type parser struct {
}

should not turn into

// MyFancyParserInterface ...
// This MyFancyParserInterface works in the way y
type MyFancyParserInterface interface {
}

As it is now it fits my needs fairly well. To make the doc 'look' like it's nice I just pass -y "IParser is an interface for Parser ..." - that way the following doc looks sort of normal even if it refers to another type in places.

Personally I'd at least want an option to have it disabled to avoid unwanted replacements.

What if we do the following?

-s mySruct -i MyInterface -D -y "MyInterface is an interface for myStruct"
- replace myStruct with MyInterface except in -y
-s mySruct -i MyInterface -D
- replace myStruct with MyInterface
--fix-type-doc=false (alias -r for replace, default true)
- no replacement, as it is now

vburenin commented 5 years ago

Oh guys, long discussion while I was asleep ;)

Updating doc makes sense, but lets move it into different PR as any other fixes. This PR gets to large to review and documentation update looks like going to be significant.

For confusing name replacement part: lets make sure your use case and original use case are covered. There are always non standard situations especially when it comes to the human generated data like comments that may complicate life and going to far may not be even needed.

DenLilleMand commented 5 years ago

@markus-wa i think that is a nice suggestion, i would like the possibility to just replace all of the struct names with the interface name at least. And disabling this kind of functionality would be good as well i think.

markus-wa commented 5 years ago

Thanks for merging this so quickly @vburenin!

@DenLilleMand I'll look into adding the other discussed option and create a new PR if i can get it to work nicely. 🙂

vburenin commented 5 years ago

@markus-wa Thanks for active and extensive contribution.

While it works well I am fine with it. You guys started scratching a deeper ground that in my opinion should be moved out to a separate thread, that's why I intentionally merged it to ensure there are no more commits into this PR ;)

vburenin / ifacemaker