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

docs: Fix ZSON named type example #5000

Merged
merged 1 commit into from
Jan 22, 2024
Merged

docs: Fix ZSON named type example #5000

merged 1 commit into from
Jan 22, 2024

Conversation

mattnibs
Copy link
Collaborator

No description provided.

@mattnibs mattnibs requested review from philrz and a team January 19, 2024 22:58
Copy link
Contributor

@philrz philrz left a comment

Choose a reason for hiding this comment

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

Thanks for catching this @mattnibs.

I just took a brief tour of the history of changes in this area since I find it helps illustrate the value of mdtest-protecting the docs whenever we can. The syntax you're correcting here was first changed in the tooling to (port=<uint16>) with the changes in #3458. Then the (port=uint16) syntax you're now correctly showing arrived with the changes in #3498.

For readability I know not every example in the docs lends itself to the full mdtest treatment, but as long as they have to exist, maybe there's something else we can do to help make sure they get updated. For instance, while the relevant part of the zson.md didn't get touched in those two PRs, the tests certainly did. Would it help at all if we added comments to one or more of those test files effectively calling attention "Hey, if you find yourself updating this, these other parts of these docs probably need the same treatment"? Or does everyone only do bulk sed-style test updates such that comments would not be noticed? I'm curious what others might think (cc: @nwt).

@mattnibs
Copy link
Collaborator Author

@philrz maybe for these kinds of tests we want to add a mdtest boomerang flag that 1) verifies it can serialize the value and 2) deserialize it to zson and 3) ensures the input/output values match?

@mattnibs mattnibs merged commit c9cf7f1 into main Jan 22, 2024
4 checks passed
@mattnibs mattnibs deleted the docs-zson-named branch January 22, 2024 18:43
@philrz
Copy link
Contributor

philrz commented Jan 22, 2024

@mattnibs: I like your idea! We had an existing issue for this class of mdtest improvement so I just added your idea as #2908 (comment). If someone wanted to knock that out as a palate cleanser I would not fight it since these little glitches in the docs have a way of sneaking up on us now and then and it bums me out to think of users that have taken the time to read the docs getting confused. But it's not a crisis if we leave it for later. The worst that happens is someone pings us on Slack.

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.

3 participants