-
-
Notifications
You must be signed in to change notification settings - Fork 676
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
Stardoc generates extras.md #2992
Conversation
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
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.
@cvarier, thanks so much for sending this in. This is a great start on the process of modernizing the documentation in rules_go!
docs/BUILD.bazel
Outdated
[core go rules]: core.rst | ||
""" | ||
|
||
# Workaround https://github.com/bazelbuild/stardoc/issues/25 |
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.
Hrm... looking at the linked issue, it's possible that it is actually resolved and the workaround isn't necessary. See alexeagle/stardoc@586650b.
If you are 100% sure that this is necessary, what would you think about writing the template.vm
file out in the //docs
directory itself and just referring to it rather than generating? I don't see anything in here that's only knowable at build time, so I would like to understand the advantages you see in generating the file instead of checking it in.
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.
Yes, you are totally right about this, thank you for pointing it out!
docs/BUILD.bazel
Outdated
------------------------------------------------------------------------ | ||
""" | ||
|
||
_NAV_FOOTER = """ |
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 purpose of these in .rst
files is to make it possible to conveniently link many times to the same place, sort of like a reusable consts. While these values defined like this are kind of nice to have, it seems to me that since none of them are used more than once, we should probably just inline the link in the .bzl that is being used to generate.
I think that would give us all the content we need on the page and simplify the BUILD file a great deal since we wouldn't need to generate a template at all.
What do you think?
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.
Good point, after a bit of finagling I was able to get it to work without the template and by transferring over the consts to docstring comments.
docs/BUILD.bazel
Outdated
name = file.replace(":", "_") + "_doc", | ||
out = file.replace(":", "_") + ".md_", | ||
header_template = ":header.vm", | ||
input = "//%s.bzl" % file, |
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.
Please move the //
to the dictionary above. I was staring at that for quite a while trying to figure out if it was a target or if this was some additional syntax that I needed to learn.
docs/BUILD.bazel
Outdated
) | ||
|
||
_DOC_SRCS = { | ||
"extras:embed_data": "//go:extras.rst", |
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.
What is the //go:extras.rst
supposed to be? I thought this was generating extras.md
?
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.
Yes, you're right, updating this.
docs/BUILD.bazel
Outdated
] + [ | ||
"cp -fv bazel-bin/docs/{0} {1}".format( | ||
k.replace(":", "_") + ".md_", | ||
v[2:].replace(":", "/").replace(".rst", ".md"), |
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.
Here and above. This feels like a lot of work to avoid writing .md
in the _DOC_SRCS
dict. I think you should just write the file that you want to compare against instead of doing some translation at the comparison point.
@achew22 Thank you for your review! I addressed your comments and updated the PR again, let me know what you think! |
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.
Two little ones then I think we can merge this
.bazelci/presubmit.yml
Outdated
@@ -327,3 +327,6 @@ tasks: | |||
- "-//tests/legacy/test_chdir:go_default_test" | |||
- "-//tests/legacy/test_rundir:go_default_test" | |||
- "-//tests/legacy/transitive_data:go_default_test" | |||
# Stardoc produces different line-endings on windows, | |||
# so the documentation it generates doesn't match the checked in files |
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.
Sorry, one more thing. Can you add a comment here identifying the issue tracking this so we can recover it when that's resolved
@@ -1,95 +0,0 @@ | |||
Extra rules |
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.
Please don't delete the file, instead leave a stub file that says "This file has moved, please look at it here"
@achew22 updated again |
@cvarier, thanks for the great PR! This is a big win for the project and progress for the whole ecosystem. |
What type of PR is this?
What does this PR do? Why is it needed?
This is a mirror of the content in extras.rst, allowing us to manually inspect the delta between them:
LHS: https://github.com/bazelbuild/rules_go/blob/master/go/extras.rst
RHS: master...aspect-dev:stardoc-chai#diff-facb0b3aafba052f3e5fe16fbe02d2c2f6b2c2c30c443fd08f4617d4c5d38532
As soon as we are satisfied that the generated content is correct, we'll delete extras.rst and update any links to it.
(This could happen as part of this PR, or subsequent)
Technique:
First step in addressing #2437
Remaining steps will be to rinse-and-repeat for other .rst files containing rule APIs
Which issues(s) does this PR fix?
Fixes #2437