-
Notifications
You must be signed in to change notification settings - Fork 16
Fetch readme and generate Markdown for CLI flags at build time #113
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
base: master
Are you sure you want to change the base?
Conversation
|
The rich text headings + monospace blocks is actually quite good. It makes the docs scannable while preserving the exact CLI output format. I'd keep this. About the em dash issue, a few alternatives come to mind:
Have you tried to use Astro's integration system instead? Create a proper Astro integration that runs during the build: // astro.config.mjs
import { generateCliDocs } from './src/integrations/cli-docs.ts';
export default defineConfig({
integrations: [
generateCliDocs(),
]
});// src/integrations/cli-docs.ts
import type { AstroIntegration } from 'astro';
import { generateCliOptionsMarkdown } from '../fetchReadme';
import { writeFileSync, readFileSync, rmSync } from 'node:fs';
export function generateCliDocs(): AstroIntegration {
return {
name: 'generate-cli-docs',
hooks: {
'astro:config:setup': async () => {
const docTemplateFile = "src/content/docs/guides/_cli.md";
const docOutputFile = docTemplateFile.replace('_', '');
rmSync(docOutputFile, { force: true });
const usageText = await generateCliOptionsMarkdown();
const docTemplateText = readFileSync(docTemplateFile, "utf-8");
const docOutput = docTemplateText.replace(
'README-OPTIONS-PLACEHOLDER',
usageText
);
writeFileSync(docOutputFile, docOutput);
}
}
};
}The main benefit would be that the dev server would regenerate the docs on restart, and you wouldn't need to manually run a script to update the docs. |
|
@mre good point. I hadn't considered using Astro's integration API to add this functionality. Because you mentioned that, I started going through community made integrations and came across this https://github.com/natemoo-re/astro-remote#README which enables "render remote HTML or Markdown content in Astro with full control over the output." what do you think? |
|
I've made changes following @mre's suggestions. We disable smart dashes across the entire site and we use a custom integration to generate the file. The integration code is basically the same as mre's suggestion, but we have to add addWatchFile to reload when the md file is changed. |
|
@katrinafyi Cool, I really like that. Rebasing on top of master should fix the link check errors. |
src/fetchReadme.ts
Outdated
| import { readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; | ||
| import { basename, dirname, join } from 'node:path'; | ||
|
|
||
| const VERSION = "lychee-v0.21.0"; |
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.
Updating this with each release manually is quite cumbersome. Instead, we can fetch the latest version via API.
# first element seems to be "nightly", second is latest stable release
curl "https://api.github.com/repos/lycheeverse/lychee/releases" | jq '.[1] | .name' -rThere 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 remember being told, effusively, by @jacobdalamb that there is a CI workflow to periodically check the release tags. That workflow doesn't exist anymore - I don't know what happened there.
My plan was that the version would be hard coded and the CI check would be a gentle reminder to update it.
I think it is good to tie the website's source code to a particular version, so that any commit can be checked out and the website can be built as it was at that point in time. This helps, e.g., when bisecting (tho this is more important for code rather than docs).
Updating the version manually could also be a reminder to update the docs with changes from that version. In the past, I have been sloppy and forgotten to write/fix the docs :)
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 deleted it because it felt tacky but I can add it back.
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 see. Let's see what people think about hard coding vs fetching the latest automatically, we might not have to bring it back.
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.
Agree, it makes sense to pin a specific release version in the code. I would opt for this approach. But then it would be really great to have automation, similar to dependabot, which checks for the newest release version and create a new PR to bump the version in the code automatically. This can be done in a later PR.
In any case it would make sense to extract this pinned version into a separate file. Possibly even a non-TS config 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.
I've pulled the version into a separate TS file. The only reason it's typescript is so I can use javascript comments, otherwise i'd have to do some manual parsing. Let me know if that works :)
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.
Sure, that works 👍
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 see what people think about hard coding vs fetching the latest automatically, we might not have to bring it back.
I personally much prefer fetching the latest automatically. It's one less thing to remember and one less thing to continuously update manually. And I don't see much of a downside. On top of that, it's less code to fetch the version instead of maintaining the GitHub workflow. @jacobdalamb @thomas-zahner wdyt? 😅
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.
Hmm, initially I thought the same. But fetching the latest version means fetching the latest version at build time. (correct me if I'm wrong) So the result purely depends on when the docs are built. If we pin a specific version in the code the docs become reproducible, meaning the same result for everyone for each revision.
Regardless of which approach we use we need to rebuild the docs, when we want to reflect changes in the README. So ideally we should write an additional workflow which handles this. (I can do so after we have merged this PR)
The main difference is that one approach keeps the result reproducible while the other doesn't. So I think pinning is just an additional "feature" we ideally should do but it doesn't affect the workflow situation. (on new release -> [optional: update pinned version in code] ->rebuild and publish docs)
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 see your point. That's fine with me.
|
I've added some colourful boxes. However, it's pretty naive and they're not quite valid examples - they still use placeholder variables and, in the case where a flag requires another, it does not show the flags being used together. The more I work with this, the more I think that it would be nice if the docs site shows the CLI help as markdown formatted text. Initially, I was reluctant because it means the CLI help text would have to be written with a particular formatting which is only useful to the website. But maybe it's not so bad because it's already most of the way there and the hard wrapping of lines is already a requirement with the code block approach. I'll investigate. The CI pipeline that builds astro is failing at the astro build step. Idk why because the package should exist as a transitive dependency, and it works on my machine with an |
This reverts commit 0936b31.
that this is generally nicer to read. However, this means that the website text is rendered from the same text as the CLI --help. The website understands Markdown, but the CLI does not (obviously) so we have to be careful. I would suggest that when writing help, the focus should be on readability within the *console*. This means that while Markdown can be used, it should be limited to syntax which is unobtrusive in raw text. Raw text is still the main format which the help text will be rendered in. Personally, these would be okay: - single `*` or `_` for emphasis (with preference for `*`) - single backticks for inline code - four space indentation for code blocks - bullet and numbered lists Imo, these would *not* be okay, because they appear too jarring in plain text: - code blocks with triple backtick fences. this includes any astro-specific asides and the like. - link syntax with `[link](https://url)` - bold when used for subheadings like `**Note**:` I think this is a good compromise which lets the same text be usable for both CLI --help and the website's rich text HTML.
|
Update to my last comment, I've gone ahead and made it markdown text rather than code blocks. See commit for commentary de6c2a8
This generally works fine and looks nice, aside from --files-from: If this is something we want, I'll PR to tweak the files-from help text. |
|
Check if |
|
It exists in my node modules after I use npm ci. Should I be using npm or pnpm? Is there a difference? Idk pnpm, so it would be good to have some commands in the readme. |
|
Use PNPM. I've added back the commands in the readme. |
src/components/code.astro
Outdated
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.
Where does that whitespace come from?
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 don't know! a past life (branch), perhaps.
| import { LYCHEE_VERSION } from "./lychee-version"; | ||
|
|
||
| // https://raw.githubusercontent.com/lycheeverse/lychee/master/README.md | ||
| const url = `https://raw.githubusercontent.com/lycheeverse/lychee/refs/tags/${LYCHEE_VERSION}/README.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.
Keeping this in sync with upstream is a bit cumbersome.
I was contemplating whether we could fetch it at build-time?
async function getLatestLycheeVersion(): Promise<string> {
const response = await fetch('https://api.github.com/repos/lycheeverse/lychee/releases/latest');
const data = await response.json();
return data.tag_name;
}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.
We talked a bit about this in #113 (comment)
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.
Thanks. My vote goes to fetching it. The "project" is the state of truth, not GitHub CI. Will add a comment there.
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.
Either way, no hard blocker. Just wanted to raise this again. We can merge however we decide.
|
I really like the new formatting! |
Co-authored-by: Matthias Endler <[email protected]>





It looks like this:
This implementation works but there are fun details to think about:
Todo: