Include constructors, methods, constants, and variables

[chrisfenner]

Hi, and thanks for building goreadme!

I think something like goreadme is sorely needed for Github Go projects - GitHub is great at rendering markdown with the code, and (IMHO) not every small open-source project needs to have its godocs hosted somewhere for useful reference. At the very least, lots of projects are in some phase of flux where they are being developed but such godoc infrastructure has not been built, and goreadme really helps document such projects.

I'm using it to generate readmes for all the packages in my project, and I've extended it for my own use:

• Include factory functions with types (go/doc defines funcs on types in the AST as "sorted list of functions returning this type")
• Include methods with types
• Include consts and vars with types
• Include const and vars with packages

An example of a package that now gets factory functions and methods is my project's normpolicy package. I don't have a super-great consts/vars example, but you can see how it works in my project's policypb package, which is generated by the protocol buffer compiler. (Which I check in rather than build just-in-time, so that go build Just Works...). I do find that having all the generated types and constants is useful for quick lookup during development.

I added another Markdown template for printing out values (from consts or types). I wasn't sure how or whether to map "printing the constants and the variables" to a config feature for turning on and off, so package-level constants and variables are now always printed, and type-level constants and variables are printed if -types is specified.

[posener]

Hey,

Thank you for this PR, it looks great.

Could you be kind and add tests? You can add them to the pkg1 test case, and check that the generated README file is what you expect.

Note that you don't need to modify the readme files manually - you can run WRITE_READMES=1 go test ./... and check the changes on the readme files (using git diff) and see that this is what you expect.

[posener]

You can check the just newly added testing section in the readme file https://github.com/posener/goreadme#testing.

Thanks!

[chrisfenner]

Thank you for adding https://github.com/posener/goreadme#testing, it made adding tests a breeze. I tried to exercise all the new code paths as simply as possible. To cover the changes to types, I modified pkg8 as well as pkg1.

[posener]

Sorry, I missed something...

Can you also add these as optional, since we don't want to break any existing readmes.

Please add them to the Config, similar to Functions I guess, add them to the command line, and add the config conditions to the template, similar to this.

Thanks!

[chrisfenner]

It's no problem - I was not sure whether you would want for any of this new behavior to be on by default, so I guessed at first 😀. From your comment, it sounds like you would prefer for these new features to be opt-in, and I appreciate that.

I added several new options to Config:

• Consts
• Vars
• Factories
• Methods

Consts and Vars affect both the package-level and types-level content. Factories and Methods only affect types-level content (so they have no effect if Types is false).

To enable fetching the configuration values from the Types sub-template (which is invoked by the main template), I converted the Config (which used to be attached to the pkg struct passed into the template) into a func. I spent quite a bit of time trying other approaches to this problem, but it seems that gotemplate is not designed to allow passing either multiple-valued inputs (such as type + config) to templates, or setting global variables (e.g., a global config variable) that are accessible from invoked templates. I hope you will agree that turning .Config into a func is not less readable and will enable future development of options that affect the sub-templates.

⚠️To allow the references to Config without causing circular dependencies, I moved goreadme.Config into internal/config.Config I then made goreadme.Config a type alias of config.Config and preserved the public API in terms of goreadme.Config to avoid breaking callers. I believe since the current test interacts with that type by the goreadme.Config spelling, this is already covered by goreadme_test.go (which I did not modify in this PR).

I have added two more tests cases (10 and 11) to cover the new behavior and test that, e.g., enabling Consts and Vars but not Types has the expected effect. I have introduced some new code into test cases 1 and 8 to help confirm that the new behavior is off by default, as you'll see that their corresponding README files have no (effective) change compared to before I introduced this feature.

[posener]

Hey, not sure why it is different than the other configurations, such as Functions. Can you explain? If you find it hard to implement, I can cherry pick your template changes (first commits) and add these flags, don't want to waste too much of your time.

[chrisfenner]

Sure, let me try to explain a little better -

The current code passes a pkg into gotemplate: https://github.com/posener/goreadme/blob/47914880175fa031f3d23e3f2116ceb107f42b4c/goreadme.go#L188-L192

template.Execute passes it along to text/template.Template.Execute, where the pkg becomes data. main.md.gotempl exposes the contents of that variable as .Config. There are some if statements in that template that check config values already.

In that file, the types.md.gotmpl is invoked on .Package, which is a *doc.Package.

{{ if .Config.Types }}
{{ template "types" .Package }}
{{ end }}

That type naturally has no idea about goreadme.Config. So, there are some options:

1. Define some type to hold a *doc.Package and goreadme.Config together, and copy the Config into that type too.
2. Flatten the types template so that it is also called on the goreadme.pkg instead of the *doc.Package. (So, it would be invoked like {{ template "types" . }} instead.).
3. Make the config available to all the templates regardless of what type they take, by putting the Config in a closure passed to the template as another function.

The latest commit here goes with (3) - what do you think?

[posener]

Thanks for the explanation.

I still don't understand what is the problem that you were trying to solve? The fact that internal/template.Execute was receiving a data interface{} and not a concrete type? If yes, I see the issue, but your fix still keeps the Pkg and SubPackages passed as interface{}. Theoretically your change could work without adding the new package and type, just like it worked before.

[chrisfenner]

Nope, sorry. The reason the change did not work without changing how Config works, is that the types template does not recieve a pkg type. It receives Package, one of the members of pkg. It can't access the Config because that is not part of the struct that's passed to the template.

An alternative option would be to refactor the types template so that it also accepts the same pkg struct as the main template. Would you prefer me to try that?

[posener]

Oh, I now see what is your issue.

See possible solution in https://github.com/posener/goreadme/commit/8fca2c15885b612413158c1e4a4ef8b8eaed3336

I think it is a bit more elegant.

I just passed . to types.md.gotmpl, and there you can use $.Config to access that top level field of the data that was given to the template.

Another option, is to use the config template function that you introduced (That I actually like), just keep it as an interface{} and then you don't need to create the internal package.

Also - I think it makes things a bit more elegant to split the values.md.gotmpl into two templates - consts and vars. It is more consistent with the other templates, and the code becomes simpler. But up to you on this issue.

Sorry for the long conversation! Really appreciate your contribution on this.

[chrisfenner]

I also like passing the config as a closure - it allows templates to be specialized to the subtree of the AST as appropriate. I'm glad that you liked that.

just keep it as an interface{} and then you don't need to create the internal package.

This blew my mind a little bit, because I underestimated the reflecty-ness of gotemplate. Awesome suggestion.

Also - I think it makes things a bit more elegant to split the values.md.gotmpl into two templates - consts and vars. It is more consistent with the other templates, and the code becomes simpler. But up to you on this issue.

I like this thinking too, and I have taken it a step further. I split values into consts and vars, with a version for "package consts/vars" and another version for "types consts/vars". They only differ in two # symbols for heading depth but I think the duplication makes for easier reading. Let me know if you don't prefer that.

Also sorry for the long conversation - I sent this PR to try to enhance your tool and save you some work and it looks like it was lots of work to review. But I enjoyed collaborating with you on this. And once again, thanks for building goreadme!

[codecov[bot]]

Codecov Report

Merging #108 (635e876) into master (a049525) will decrease coverage by 0.37%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage   69.51%   69.13%   -0.38%     
==========================================
  Files           3        3              
  Lines          82       81       -1     
==========================================
- Hits           57       56       -1     
  Misses         15       15              
  Partials       10       10              

Impacted Files	Coverage Δ
goreadme.go	66.66% <ø> (-0.69%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a049525...635e876. Read the comment docs.

[posener]

Also sorry for the long conversation - I sent this PR to try to enhance your tool and save you some work and it looks like it was lots of work to review. But I enjoyed collaborating with you on this. And once again, thanks for building goreadme!

Thank you! This PR is a great example for open source collaboration and contributions. I'll buy you a beer if you'll ever make it to Tel Aviv :-)

[chrisfenner]

Woohoo, found some time to fix the nits!

I appreciate it! If you find your way up to Seattle, likewise!

[posener]

:beers: