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

clients: overhaul docs generation #1417

Merged
merged 4 commits into from
Jan 12, 2024
Merged

clients: overhaul docs generation #1417

merged 4 commits into from
Jan 12, 2024

Conversation

matklad
Copy link
Member

@matklad matklad commented Jan 10, 2024

TL;DR: turn the logic for generating client readmes upside down --- rather than assambling a readme sample out of code fragments from docs.zig, write the entire readme sample as a proper sample in the samples directory, and inject fragments of that sample into the readme.

Motivation: right now, we have two completely different code paths for integration-testing our client samples:

  • In the ci scripts, we run every sample for every client. This is triggered by clients.yaml on CI.
  • In docs_generate we asseble a sample from our readme fragments and test that. This used to be tested by our CI, but got semi-intentionally dropped during the latest overhaul.

This PR reconciles the two approaches, by making sure that the code from README is just a sample. This also has auxilary benefits:

  • we now can reformat the code from the samples. This is not automated, but I did a run through with a formatter manually.
  • Now the code from readme can be conveniently debugged, as it exist on the file system
  • Future work: with some effort, I think the walkthrough sample might become a reasonable sample for new users to directly look at
  • Future work: now it is possible to add more thourghou checks to the sample, which are not necessary shown in the readme, but could test that the behavior is what we expect (as written, our samples return a ton of errors like "account id should not be zero").

I tried to change as little as possible, as the diff here is large anyway. I do plan to send a significant cleanup PR as a follow up: for example, I believe the entirety of shutil.zig and run_with_tb.zig is now dead code.

That said, there's a bunch of user-visible changes here already:

  • Code in readme is now formatted! E.g., .Net now puts { on a line by itself.
  • From java, and .Net, I dropped "don't forget to close the client" in favor of just showning the try-with-resources version, I don't think we should advertise bad practicies, and it was simpler to just drop this altogether, rather than add it to walkthroughs.
  • Before, we were also validating that "shell" bits of readme are correct. Thigs like npm install tigerbeetle-node and node main.js. Now, these are not tested. I think this is fine --- these are upstream stable interfaces which are not going to change. We might have some bugs there, but the cost of such bug feels small, and they are unlikecly to reappear after they are repaired.

@matklad
Copy link
Member Author

matklad commented Jan 12, 2024

The end state I am aiming at with this stack of PRs:

#1422

@@ -0,0 +1 @@
Code from README collected into a single runnable project.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe link to the README that is being referred to (since this file is also a README).

Copy link
Member Author

Choose a reason for hiding this comment

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

since this file is also a README

🤦

const section_end = self.sprintf("endsection:{s}\n", .{section_name});

var text = self.walkthrough;
for (0..10) |_| {
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading this right, if there are multiple sections with the same name, then this concatenates them all together, is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct! This is used for java, which, for the first, imports example, needs to concatenate public static void main(String[] args) { with the } at the end of file.

added a comment!

@matklad
Copy link
Member Author

matklad commented Jan 12, 2024

😩 docker:

Unable to find image 'alpine:latest' locally
docker: Error response from daemon: Head "https://registry-1.docker.io/v2/library/alpine/manifests/latest": Get "https://auth.docker.io/token?account=githubactions&scope=repository%3Alibrary%2Falpine%3Apull&service=registry.docker.io": EOF.
See 'docker run --help'.

Amusingly, googling this error turns up some results from January last year!

These samples just collect code from README.md into on runnable project.
The idea is that we will _generate_ README from this sample project. As
a side effect, readme gets tested through the usual samples-testing
infra.
The "walkthorough" sample is now validated together with all the other
samples.
@matklad
Copy link
Member Author

matklad commented Jan 12, 2024

Ok, for some reason, re-running the job didn't work, but rebasing the whole branch did the trick.

@matklad
Copy link
Member Author

matklad commented Jan 12, 2024

Wait, somehow, after a rebase, the review is not dismissed? 🤔 this seems like a new behavior...

@matklad matklad added this pull request to the merge queue Jan 12, 2024
@sentientwaffle
Copy link
Member

Wait, somehow, after a rebase, the review is not dismissed? 🤔 this seems like a new behavior...

I noticed that too! I wonder what the heuristic is for whether or not the approval is retained.
https://github.com/orgs/community/discussions/12876#discussioncomment-7445850 is the only info I've found.

Merged via the queue into main with commit 58d878a Jan 12, 2024
25 checks passed
@matklad matklad deleted the matklad/docs-generate-II branch January 12, 2024 18:07
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.

None yet

2 participants