Repository navigation
add documentation guidelines - #2083
Conversation
There was a problem hiding this comment.
Nice, useful guideline — just needs one contradiction with its own build change resolved before merge, thanks!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=cec5e750-d993-4668-8c26-dd64fe14a590
There was a problem hiding this comment.
Thanks for pulling these rules together — just a few concrete fixes inline before this can go in.
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=f527bdd9-6f7f-4bfd-afcc-d591e2753761
Allex-Nik
left a comment
There was a problem hiding this comment.
Great guidelines and skill!
I left some comments with suggestions
zaleslaw
left a comment
There was a problem hiding this comment.
Review: documentation guidelines and the operation-website-documentation skill
Checked at f94939da4 against the repo: DOCUMENTATION_GUIDELINES.md,
.agents/skills/operation-website-documentation/SKILL.md, and the edits to README.md, docs/README.md,
docs/StardustDocs/AGENTS.md, samples/AGENTS.md, samples/build.gradle.kts.
Most of the guide holds up. Every file path and anchor exists (DataRow.md#row-expressions,
#row-conditions, ColumnSelectors.md#column-resolvers, DataColumn.md#columngroup, …), and so do
DocumentationUrls, groupSamples, DataFrameSampleHelper, issue #898 and the Korro/Kandy/Algolia links.
All of them are described correctly. Five things need fixing before merge. Items marked (already raised)
were commented on earlier in this PR but are still open at this commit.
Blocking
1. The links to the skill are broken. The last commit moved the skill to .agents/skills/…, but four
files still link to .claude/skills/operation-website-documentation/SKILL.md, which no longer exists:
README.md, docs/README.md, docs/StardustDocs/AGENTS.md, samples/AGENTS.md.
Please point them at .agents/skills/…. Claude Code discovers skills only under .claude/skills/, so it also
needs a pointer from there (as suggested above: keep the skill in .agents and link to it from .claude).
Without that pointer, Claude Code won't find the skill.
2. The PR contradicts itself on _accessors (already raised). The PR removes funSuffix("_accessors")
from samples/build.gradle.kts, but the suffix is still listed in DOCUMENTATION_GUIDELINES.md:99 and :323,
in SKILL.md:137, and in samples/AGENTS.md:34 and :46. Removing it is safe: no *_accessors test exists
in samples/src or in any topic. Only the text needs updating.
3. Signatures in a kotlin block conflict with docs/StardustDocs/AGENTS.md:52-54 (already raised).
That file says: "Never hand-write a ```kotlin block in a topic… Only text-fenced pseudo-grammar". This PR
edits the same file but leaves the rule in place, and `groupBy.md` and `pivot.md` already use `text`.
Please switch "Signatures" (`DOCUMENTATION_GUIDELINES.md:177-186`) and `SKILL.md:86` to a `text` block. While
there, it would help to add how to write an infix signature (`df1 concat df2`), as asked above.
4. The recommended templates don't follow the guide. SKILL.md:45-49 names valueCounts.md,
countDistinct.md, sliceRows.md and filter.md as templates and says to "mirror their structure". None of
them has summaries, "See also", ### Parameters or ### Examples. valueCounts.md also uses a bold
**Parameters:** paragraph, which the guide forbids, and has its signature in a kotlin block. An agent will
copy exactly that. Either bring one page in line and name it as the reference, or say explicitly: "take only
the mechanics from the template (IMPORT/FUN/iframe, the test class); take the structure from the guidelines".
5. Nothing says what to keep after clean build test korro. A local run rewrites topics and
resources/ for every page, not just yours. Per the root AGENTS.md, the CI bot commits
docs/StardustDocs/topics/ and resources/snippets/ itself. The PR has to carry two things on its own: the
iframe HTML outside snippets/, and _shadow_resources.md. Suggested step:
Before the run, save
git status --porcelain. After it, keep your topic, your test, your HTML files and
_shadow_resources.md, andgit checkout --everything else.
Inaccuracies
| Where | Issue | Suggested fix |
|---|---|---|
Guidelines :394-396, SKILL :204-207 |
clean only wipes build/, where korroOutputLines lives. HTML files of renamed or deleted samples in docs/StardustDocs/resources/ survive it |
Add: "delete stale HTML by hand, then run updateShadowResources" |
Guidelines :393, SKILL :199 |
build already runs test, and korro already dependsOn(tasks.test) (samples/build.gradle.kts:82-84) |
:samples:clean :samples:build :samples:korro. Replace "korro must run after test" with "korro runs the tests itself" |
Guidelines :398 |
The task's module isn't named | :samples:updateShadowResources (samples/build.gradle.kts:194) |
Guidelines :236-244 |
The template says "Discover … operation for Kotlin DataFrame", while 11 existing pages say "in Kotlin DataFrame" and only 2 say "for" | Use "in" |
Guidelines :228 |
writerside/inject-summary.html returns 404 |
https://www.jetbrains.com/help/writerside/semantic-markup-reference.html (has web-summary) |
SKILL :224-225 |
"path relative to the current topic" contradicts Guidelines :57 ("just the file name") |
Use the file name. That's how 108 of the 112 links to DataRow.md are written |
SKILL :17-21 vs :232-239, :253 |
The hard rules list only topics, sample tests, d.tree and build.gradle.kts as hand-editable, but steps 6 and 7 edit DocumentationUrls.kt (core) and delete functions from :core tests |
Add both paths to the list |
SKILL :83, :91 |
Links like docs/StardustDocs/... don't resolve from .agents/skills/…/ (:133-134 correctly use ../../../) |
Prefix them with ../../../ |
Guidelines :340-341 |
The guide says the output file is named after the test method, but .saveSample() writes Class.method (DataFrameSampleHelper.kt:27-36) |
Clarify |
docs/contributions.md, CONTRIBUTING.md:51 |
The old guide is still there and tells contributors to put samples into core (Modify), and CONTRIBUTING.md still links to it (already raised) |
Replace the old guide with a link to the new one, and update CONTRIBUTING.md:51 |
| Guidelines | Typos: suffices (:99, :323), "replace remove" / "wrote" / "has" (:363-369, already raised), pathes (:391, already raised), :sample module (:393), wesbite (:399), "save into" (:291) |
suffixes, remove, written, have, paths, :samples, website, saved into |
Suggestions: rules worth adding
- Assertions in samples (already raised: "Do samples need assertions?"). Korro keeps the output fresh,
but it doesn't check that the output is right: a sample without an assertion stays green whatever the
behaviour. Suggested rule: ashouldBeafter// SampleEnd, in the same test. Korro doesn't inject it, so
the page stays clean. Precedent:samples/.../api/Read.kt. - One example, three places. The KDoc, the page and the test use the same dataset and the same calls.
This also answers the open question about aligning@param/@returnin KDoc with the website. - Each
##section stands on its own. Overview pages link straight to anchors. So alongside "show the
dataset once per page", later sections need one line pointing back to that data. - An example that demonstrates a rule should tell it apart from the rule a reader might assume instead.
If the output would look the same under both, the example shows nothing. (A sort-order example keyed by
first name inpeopleDfcan't distinguish first-occurrence from last-occurrence order; keyed by city it
can.) - No GitHub links on website pages, neither to issues nor to source files. Describe a known bug in plain
words, and keep the issue number in the test'sTODO. - Keep sample lines under ~85 characters, so a reader doesn't have to scroll sideways on the page.
- Link a new page from its group's overview page (e.g.
info.md), not only fromd.tree. - Rephrase the "do not" rules as positive ones (see the comment on
SKILL.md:114). For example, instead
of "don't write which overloads are missing", write "describe only the overloads the operation has".
There was a problem hiding this comment.
Thanks for pushing this forward — the guide reads well now; a few self-contradictions inline need resolving before merge.
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=da30c4a7-3fb9-4b84-9736-170dc46cfaeb
There was a problem hiding this comment.
Genuinely useful guideline — two blocking points inline to settle before merge, thanks!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=42d0da08-9a16-4596-a880-6b26be7c65df
There was a problem hiding this comment.
Thanks for pushing this forward — a few blocking fixes inline and it's good to go!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=b381e26c-ed88-494f-9f8f-001228b29e98
|
Requires #2155 |
Jolanrensen
left a comment
There was a problem hiding this comment.
Looks good and helpful! some small notes
…d update links in documentation and samples accordingly. Extend documentation guidelines with linking rules, usage examples, and mandatory steps.
…rected typos, and clarified sample migration steps
… and focusing on present method overloads.
…rove consistency, and update guidelines for snippet generation and column accessors.**
…nd guidelines** Correct typos, align column group and sample output instructions, and clarify edge case examples. Refine `inline-frame` usage and file path handling for nested and flat resources.
- Automate insertion of `<inline-frame>` and `<img>` into `FUN`/`END` blocks via Korro. - Clarify rules for dropping `_properties`/`_strings` suffixes in output file names. - Remove dependency on manual iframe placement; ensure consistent flat naming in `src` links. - Update documentation examples and guidelines to reflect these changes.
e592f84 to
c73fc6e
Compare
There was a problem hiding this comment.
Thanks for all the iterations here — one blocking point inline to settle, and this is good to go!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=92b43fbc-b89a-4271-9463-8021e44317ba
| @Test | ||
| fun filter_strings() { | ||
| // SampleStart | ||
| df.filter { "age"<Int>() > 18 && "name"["firstName"]<String>().startsWith("A") } |
There was a problem hiding this comment.
Blocking: the reference sample uses the one API this PR forbids.
"age"<Int>() / "name"["firstName"]<String>() is the Invoked String API, which StringApi.md:199-205 marks outdated and "not recommended in production code". This PR bans it twice — DOCUMENTATION_GUIDELINES.md:121 ("Don't use the outdated Invoked String API") and SKILL.md:178-180, which names these exact two forms. Yet DOCUMENTATION_GUIDELINES.md:459-464 nominates filter.md + FilterSamples.kt as a reference that "fully follows these guidelines", and SKILL.md:57 tells the agent to "mirror their structure, tone and formatting".
Failure scenario: the first agent run of the skill reads FilterSamples.kt as its template, copies the "col"<T>() shape into the new page's _strings tab, and ships a page built on the deprecated API — while the same skill run also reports the guidelines as followed. Nothing in the build flags it, so it lands as the new precedent for every page after it.
Suggested fix: rewrite the _strings variant with the accessors SKILL.md:178 prescribes — df.filter { getValue<Int>("age") > 18 && getColumnGroup("name").getValue<String>("firstName").startsWith("A") } (both are non-deprecated DataRow members) — and update filter.md's generated tab accordingly. Worth noting StringApi.md:174-196 demonstrates the invoked form under String Column Accessors too, so that section likely needs the same correction for the rule to be followable.
There was a problem hiding this comment.
Really nice to see the two reference pages land — one blocking inconsistency inline to settle before merge, thanks!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=5ff86bc9-2e63-4e5d-8e1a-0e9ee353e636
| @Test | ||
| fun filter_strings() { | ||
| // SampleStart | ||
| df.filter { "age"<Int>() > 18 && "name"["firstName"]<String>().startsWith("A") } |
There was a problem hiding this comment.
Blocking: the reference sample uses the one API this PR forbids.
"age"<Int>() / "name"["firstName"]<String>() is the Invoked String API — literally the canonical example inside StringApi.md's ### Invoked String API section ("We don't recommend using it in production code"). This PR bans it in three places: DOCUMENTATION_GUIDELINES.md:121, SKILL.md:178-180, and samples/AGENTS.md. It came over verbatim from the old Modify.kt sample; the very same commit (c73fc6e) converted the equivalent ConcatSamples.kt:278/319 lines to getValue<Int>("age"), so filter is just the one that got missed.
Failure scenario: filter.md + FilterSamples.kt are named the reference template in DOCUMENTATION_GUIDELINES.md:461 and SKILL.md:49, and step 1 tells the agent to read both and mirror them. The first run reads this file and emits a _strings sample in the banned style — while concat.md, the other reference, demonstrates the opposite. The two templates teach contradictory rules, and the reviewer hand-fixes exactly what the skill exists to prevent.
Suggested fix — the row-expression accessors from StringApi.md's own table:
df.filter { getValue<Int>("age") > 18 && getColumnGroup("name").getValue<String>("firstName").startsWith("A") }plus import org.jetbrains.kotlinx.dataframe.api.getValue, and the same edit in the filter.md:74 Strings tab once korro regenerates it. Verified locally: :samples:compileTestKotlin and the FilterSamples tests pass; this sample saves no output, so no HTML is regenerated.
Allex-Nik
left a comment
There was a problem hiding this comment.
I think it's needed to fix the contradiction Air pointed out: Invoked String API is prohibited in the guidelines, but it is now used in filter.
Otherwise good.
|
@Allex-Nik it's here #2166 and we are thinking about adding both options in examples |
Closes #1690