Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generate docs #355

Merged
merged 9 commits into from
Feb 26, 2019
Merged

generate docs #355

merged 9 commits into from
Feb 26, 2019

Conversation

ruseinov
Copy link
Contributor

Work on #187

@ethanfrey I have generared sample docs in html. This plugin does not seem to support RST, so we can probably choose between .md and .html.

It looks like the easiest way is to generate docs with go proto defs and commit as a developer.
But I think this is not very usable at the moment and it'd be nicer to have a template there too.

@ruseinov ruseinov requested a review from ethanfrey February 26, 2019 09:11
@ruseinov ruseinov requested a review from alpe as a code owner February 26, 2019 09:11
@ruseinov
Copy link
Contributor Author

Also, because it's a protoc plugin - this is not a single file definition, but rather an html per proto def, same as our go definitions.

I'm really not sure how useful that is.

@webmaster128 can you please chip in?

@codecov-io
Copy link

codecov-io commented Feb 26, 2019

Codecov Report

Merging #355 into master will increase coverage by 0.3%.
The diff coverage is 53.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #355     +/-   ##
========================================
+ Coverage      70%   70.3%   +0.3%     
========================================
  Files         127     127             
  Lines        6269    6536    +267     
========================================
+ Hits         4391    4600    +209     
- Misses       1430    1462     +32     
- Partials      448     474     +26
Impacted Files Coverage Δ
cmd/bnsd/app/examples.go 0% <0%> (ø) ⬆️
app/results.go 79.3% <100%> (ø) ⬆️
cmd/bnsd/x/nft/username/msg.go 82.8% <100%> (ø) ⬆️
x/nft/approvals.go 54.6% <100%> (ø) ⬆️
cmd/bnsd/x/nft/username/handler.go 71.2% <100%> (ø) ⬆️
x/distribution/init.go 76% <0%> (-24%) ⬇️
x/currency/init.go 66.6% <0%> (-19.1%) ⬇️
examples/mycoind/app/init.go 88.4% <0%> (-3.9%) ⬇️
x/escrow/controller.go 82.3% <0%> (-3.4%) ⬇️
x/paychan/handler.go 63.7% <0%> (-1.6%) ⬇️
... and 8 more

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 dd41559...1c1ce45. Read the comment docs.

@webmaster128
Copy link
Contributor

Yeah nice! Checking this in is good as well.

and it'd be nicer to have a template there too.

It gives us 80 % of the value with 20 % of the work. This is great stuff.

This plugin does not seem to support RST, so we can probably choose between .md and .html.

html is great

Also, because it's a protoc plugin - this is not a single file definition, but rather an html per proto def, same as our go definitions.

Merging all *.proto files into one document is key since users do not want to try to understand this project's folder structure before using it. The proto files should be merged more or less the same way they are merged in the js codec generation before the html file if generated:

find . -name '*.proto' -not -path '*/vendor/*' -not -path '*/examples/*' -not -path '*/cmd/bcpd/*' | sort

@ruseinov
Copy link
Contributor Author

@webmaster128 I'm not sure how find helps.
I have tried some things, but ultimately there is a problem with trying to output docs for all the files

	protoc -I=. -I=$(GOPATH)/src --doc_out=docs/proto/ --doc_opt=html,index.html `find . -name '*.proto' -not -path '*/vendor/*' -not -path '*/examples/*' -not -path '*/cmd/bcpd/*' | sort | tr '\n' ' ' | sed -e 's/\.\///g' `

will complain
cmd/bnsd/x/nft/username/codec.proto:9:24: "username.UsernameToken.base" is already defined in file "github.com/iov-one/weave/cmd/bnsd/x/nft/username/codec.proto".

Because of double imports. It seems to be tricky and I have yet to find a way for this to work.

@webmaster128
Copy link
Contributor

I'm not sure how find helps.

In contrast to the existing Makefile, you do not need to maintain a list of folders. But feel free to use any other method to collect all relevant .proto files.

Looks like the system does not understand that

cmd/bnsd/x/nft/username/codec.proto
github.com/iov-one/weave/cmd/bnsd/x/nft/username/codec.proto

are the same file

@ruseinov
Copy link
Contributor Author

@webmaster128 I have come up with this solution:

	protoc `find . -name '*.proto' -not -path '*/vendor/*' -not -path '*/examples/*' -not -path '*/cmd/bcpd/*' | sort | tr '\n' ' ' | sed  's|\./|-I=|g'`  --doc_out=docs/proto/ --doc_opt=html,index.html `find . -name '*.proto' -not -path '*/vendor/*' -not -path '*/examples/*' -not -path '*/cmd/bcpd/*' | sort | tr '\n' ' '	 | sed -e 's/\.\///g' `

But it outputs a pretty empty doc file. I'm not sure if this is worth fighting for to be honest, I'll invest a bit more time though.

@webmaster128
Copy link
Contributor

I wonder why this does not happen when we build the .js files from the protos in IOV-Core: https://github.com/iov-one/iov-core/blob/master/packages/iov-bns/package.json#L31
Maybe because no include paths (-I) are specified?

@ruseinov
Copy link
Contributor Author

@webmaster128 golang pb generator seems to work differently.
I couldn't make this work using various techniques, so will give it up for now unless somebody has a good solution in mind.

@webmaster128
Copy link
Contributor

webmaster128 commented Feb 26, 2019

Couln't re remove all import statements that start with import "github.com/iov-one/weave?

Here is a find and patch script for .proto files https://github.com/iov-one/iov-core/blob/master/packages/iov-bns/scripts/cleaned_protos.sh that could be adjusted

@ruseinov
Copy link
Contributor Author

@webmaster128 yeah we could, I'm just not sure that adjusting the files and then generating a doc from this is the best way to approach it, because then the paths don't correspond to whatever is in the app.

Should we define a proper definition of done here first?

@ethanfrey
Copy link
Contributor

I will take a look at this today, see if I can combine them somehow.

We need the full paths in the protobuf files for the golang compiler, as it will use that as the import path in *.pb.go files , and go needs absolute paths.

I've fought protoc a bit with imports before, I think I might be able to shake this out in within an hour. If not, I would propose multiple html files and do post-processing.

@ruseinov
Copy link
Contributor Author

@ethanfrey take a look at my solution here #355 (comment) , it kind of works, but generates an empty doc (only base types) and also throws and error
[libprotobuf WARNING google/protobuf/compiler/parser.cc:562] No syntax specified for the proto file: . Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.) which comes from some vendored dirs it seems.

@ethanfrey
Copy link
Contributor

Okay, I ran make prototools to set up the needed tools, then

protoc --doc_out=docs/proto/ --doc_opt=html,cash.html --gogofaster_out=. cash/*.proto to build sample docs (from make protoc)

The output looks nice, but my first reaction (besides many small files) is that the links are broken, for example docs/proto/cash.html has a link to x.Coin that doesn't do anything.

I will try to see what I can build

@ethanfrey
Copy link
Contributor

Okay, simple case of getting x.Coin and cash.FeeInfo, cash.SendMsg all in one doc...

protoc -I=. --doc_out=docs/proto/ --doc_opt=html,cash.html x/cash/*.proto x/*.proto

Of course, I had to edit x/cash/codec.proto first to change the import:
import "github.com/iov-one/weave/x/codec.proto"; -> import "x/codec.proto";

But then,
protoc -I=. --gogofaster_out=. x/cash/*.proto is broken, with the following import:

import x "x"

@ethanfrey
Copy link
Contributor

@ethanfrey
Copy link
Contributor

I had to clean up the protoc step, there were lots of errors about not importing gogoprotobuf... it seems it was in your GOPATH, and that was being used, not the one in vendor, which is locked. Also, one regenerating there were significant diffs, as there is a different template used from the your local copy of gogoprotobuf and the one in vendor (not sure which is newer).

Anyway, now always run from vendor for more reproduceable build

@ethanfrey
Copy link
Contributor

ethanfrey commented Feb 26, 2019

@webmaster128 Now built in a single-file, hopefully this is what you were looking for.

http://htmlpreview.github.io/?https://github.com/iov-one/weave/blob/feature/proto-docs/docs/proto/index.html

Let me know if this is better.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it now, after adding some scripts for the single-file protobuf files.

@ruseinov
Copy link
Contributor Author

@ethanfrey yeah, it might have actually been an executable problem, glad that this is cleaned up now. looks good to me

@ethanfrey ethanfrey merged commit 515b07f into master Feb 26, 2019
@ethanfrey ethanfrey deleted the feature/proto-docs branch February 26, 2019 16:18
@webmaster128
Copy link
Contributor

Beautiful 😍 😍 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants