-
Notifications
You must be signed in to change notification settings - Fork 70
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
docs: begin generating docs for defs #186
Conversation
i don't know why rules_license is needed all of a sudden... |
Borked up internal setup. I fixed it, however I was waiting for internal review of the fixes. Now they are submitted, rules_proto again pass at head. So you can drop the license related modification. |
LGTM. I dislike that the docs/stardoc_with_diff_test.bzl is copied and not public in skylib. But it looks like copying is the best of options. |
yes, we have that as part of our bazel-lib at aspect, i guess i can't add that as a dev dependency here? |
No please don't. I'd then need to pull it into google or rewrite it. |
file1 = out_label, | ||
# Output from stardoc rule above | ||
file2 = out_file.replace(".md", "-docgen.md"), | ||
tags = ["no_windows"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is broken on the import. That's because we use native.starlark_doc_extract
everywhere. The latter, available in Bazel, will also generate the documetation for proto_common
builtin to Bazel (because it refers to it directly).
Usually a quick fix would be to disable the test using a tag. However I can't do this here, because the stardoc_with_diff_test
doesn't pass tags
to the difftest
.
For a quick solution I'll can comment out the test on the import.
Alternative would be that you also use native.starlark_doc_extract
. But then you'd need to update the docs on each Bazel release. Which doesn't seem so bad. Also proto_common documentation is not available anywhere I think. (So maybe it makes sense to do on a followup PR)
No description provided.