-
Notifications
You must be signed in to change notification settings - Fork 18
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
Regenerate reference docs from in-repo sources #50
Conversation
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.
It looks like there's a few places where the generation script potentially has the wrong behaviour.
@@ -559,7 +563,7 @@ Directory to use for named global caches for tools and processes with trusted, c | |||
<Option | |||
cli_repr={`--local-execution-root-dir=<str>`} | |||
env_repr='PANTS_LOCAL_EXECUTION_ROOT_DIR' | |||
default_repr={`/tmp`} | |||
default_repr={`<buildroot>`} |
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.
Is this change correct?
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.
That one should be <tmpdir>
: https://www.pantsbuild.org/docs/reference-global#local_execution_root_dir
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.
Ah yeah, this looks like a bug in the generations script, at some point I think pants_workdir
changed defaults.
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.
I think we can let this one go, it looks like a "problem" in existing docs before 2.13:
-
2.12: https://www.pantsbuild.org/v2.12/docs/reference-global#local_execution_root_dir
-
2.13: https://www.pantsbuild.org/v2.13/docs/reference-global#local_execution_root_dir
And indeed, the docs match, e.g. 2.20:
pantsbuild.org/docs/reference/global-options.mdx
Lines 695 to 696 in 09d492d
env_repr='PANTS_LOCAL_EXECUTION_ROOT_DIR' | |
default_repr={`<tmp_dir>`} |
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.
I split this into a dedicated issue: #60
To remove a prefix: | ||
|
||
``` | ||
# Results in `data.json`. | ||
relocated_files( | ||
file_targets=["src/resources/project1:target"], | ||
src="src/resources/project1", | ||
dest="", | ||
) | ||
|
||
``` |
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.
It looks like the spacing has gone a bit weird here.
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.
Possibly
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.
Fixed in #53
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.
Although, how did you re-generate the docs here? That red line should've been fixed by prettier
. Did you run prettier
after running the generation command?
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.
I didn't; I didn't realise I had to :)
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.
If there was a way to have it run in the script definition (e.g. run the generation and format) id +1 that
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.
Done in #65 (review)
(Waiting for #54 before I check over this again.) |
I've done some very quicks spot checks of the diff and of the rendering, and it seems fine. |
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.
Let's goooo!
This does a bunch of leg work to have the reference files be reformatted (with prettier) as they are written. This makes the generation notably slower (slightly less than 1s -> slightly less than 4s on my machine), but saves from having to remember to run the formatting separately... which takes just as long as the extra time anyway. This requires a few steps, broken across commits in a useful way (with some not-strictly-necessary changes), because prettier only has an async API. This seems to require running as an `.mjs` ("module") rather than `.js` to be able to use `await` at the top level. After getting to using async + prettier, this then makes a few changes that this allows: - a neater way to write the JSON files - writing all files concurrently with `Promise.all` (writing the targets and subsystems concurrently takes the time from ~4.4s -> ~3.8s, doing the others concurrently makes no difference, but makes the code look uniform). See also: #50 (comment)
This runs
for d in docs versioned_docs/*; do npm run generate-reference $d; done
,npm run format-all
to regenerate all docs off thehelp-all.json
files that are now checked in (#43).Fixes #37, hopefully.