Conversation
github-actions
bot
added
Needs tech review
|
The PR preview for e87970b is available at theforeman-foreman-documentation-preview-pr-4722.surge.sh No diff compared to the current base |
Lennonka
changed the title
maximiliankolb
left a comment
maximiliankolb
left a comment
There was a problem hiding this comment.
I am lacking some information about what this PR is about.
- Please have a look at CONTRIBUTING to see how to best write commit messages so that they align with our conventions.
- Is all text in
.claude/purely aimed at the AI? If so, does it even make sense to review it if the results based on it are good? - RE assemblies: Please have a look at our own assemblies. Almost all of them align to our own conventions. I noticed that you mention
ifdef::context[:parent-context: {context}]which we try to avoid or simplify if necessary. - RE anchors: We try to avoid appending
_{context}. - One file name uses whitespace. Any chance to rename it?
- RE nested assemblies: We generally don't do this. So I assume the recommendation to the AI would need to be adjusted.
Do you have an idea how best to review it?
As stated in the PR's description, this is a follow-up after a presentation @dvozenil did for the team a few days ago. I intend to review the PR and provide some guidance on how to align the files with our conventions; I believe these skill files could establish a solid foundation for an upstream-friendly quality review process. |
|
Yes, maybe I could have been more descriptive. These are meant more as proof-of-concept and for further tinkering by the team. The skills are originally meant for RHEL IdM docs work and reflect that – I have update the short description and cqa reviewer skills to better comply with this repo. But doc-structure-checker has not been updated for this and uses standard templates as well. CLAUDE.md is also optional to include here – it is a reference file for Claude Code and can be created by anyone using /init in the repo in a Claude Code session. But it is useful for it and contains repo-specific information, so I have kept it here. |
|
It's quite okay that the files don't fully correspond to this project's conventions and workflows... yet :) That's what a PR is -- a proposal that starts a collaborative process on getting the content aligned with the project. I have briefly scanned the skill files and will suggest a few changes to get started on aligning them with our standards. My goal as a reviewer will be to get these files into a mergeable state... and then after that, continue adapting them in follow-up PRs. Stay tuned ;) |
Lennonka
left a comment
Lennonka
left a comment
There was a problem hiding this comment.
A few observations after an initial, quick review.
I'm expecting other writers to review this more thoroughly.
aneta-petrova
left a comment
aneta-petrova
left a comment
There was a problem hiding this comment.
A few suggestions where I noticed the PR is not entirely in line with our conventions.
Co-authored-by: Lena Ansorgová <zuansorg@redhat.com> Co-authored-by: Aneta Šteflová Petrová <apetrova@redhat.com>
…xt lines - Rename checklist file to use underscores instead of spaces - Update reference in SKILL.md to match new filename - Remove nested assembly parent-context lines (assembly reuse is rare in this repo) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
CLAUDE.md is a local Claude Code configuration file and should not be tracked in the repository. It is excluded via .git/info/exclude locally. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
I think all the comments have been addressed and resolved @aneta-petrova |
|
Thanks a lot @dvozenil! @Lennonka @maximiliankolb Would you be okay with merging this? A bit more work is probably needed to fully align the skills with our workflows but I'd like to volunteer to pick up that work in follow-up PRs. For example, the Short Description Expert skill seems to produce better results than the Abstract skill that we currently have, so I'd like to take a closer look at them and combine them. Also, I see a lot of potential in the CQA Reviewer file, especially if we adapt it for our reporting needs and perhaps tie it to the other skill files that I'm working on in other PRs of my own. |
|
I'm cool with that. |
maximiliankolb
left a comment
maximiliankolb
left a comment
There was a problem hiding this comment.
I made a few comments but it's hard to review this overall because I do not use it.
If everyone else is good with it, then please focus on the comments about _{context} & the lack of IDs in assembly_*.adoc files.
|
|
||
| **Exclude from review**: | ||
| - Assembly files: `assembly_*.adoc` — abstracts live in their first included concept module, not in the assembly itself | ||
| - Attribute files: `attributes*.adoc`, `_title-attributes.adoc` |
There was a problem hiding this comment.
| - Attribute files: `attributes*.adoc`, `_title-attributes.adoc` | |
| - Attribute files: `attributes*.adoc` |
There is no file named *_title-attributes.adoc. The regexp is enough:
$ find . -name "attributes*.adoc"
./guides/common/attributes-orcharhino.adoc
./guides/common/attributes-foremanctl-satellite.adoc
./guides/common/attributes-foreman-el.adoc
./guides/common/attributes-foreman-deb.adoc
./guides/common/attributes-typography.adoc
./guides/common/attributes-katello.adoc
./guides/common/attributes.adoc
./guides/common/attributes-satellite.adoc
./guides/common/attributes-foremanctl-katello.adoc
./guides/common/attributes-foremanctl-orcharhino.adoc
./guides/common/attributes-base.adoc
./guides/common/attributes-titles.adoc
| - Follow What+Why formula | ||
| - Are 1-2 sentences, between 50-300 characters, maximum ~50 words | ||
| - Don't repeat the title verbatim | ||
| - No self-referential language ("This document describes...", "This section explains...", "The following examples...", "The examples below...") |
There was a problem hiding this comment.
| - No self-referential language ("This document describes...", "This section explains...", "The following examples...", "The examples below...") | |
| - Do not use self-referential language ("This document describes...", "This section explains...", "The following examples...", "The examples below...") |
| @@ -0,0 +1,159 @@ | |||
| --- | |||
| name: short-description-expert | |||
| description: Recursively maps AsciiDoc includes from master.adoc, audits every module's short description for content quality, and rewrites abstracts that don't meet the "What and Why" formula. Processes files in batches of 50 with progress tracking. | |||
There was a problem hiding this comment.
Does that mean we aim for PRs that change 50 files at a time? That'd be a great limit & get stuff merged faster & interate quicker.
|
|
||
| # Short description expert | ||
|
|
||
| I am a specialist in managing documentation pipelines and writing DITA/AsciiDoc short descriptions. I map document structures, evaluate existing abstracts for quality, and ensure all abstracts provide immediate user value. |
There was a problem hiding this comment.
| I am a specialist in managing documentation pipelines and writing DITA/AsciiDoc short descriptions. I map document structures, evaluate existing abstracts for quality, and ensure all abstracts provide immediate user value. | |
| I am a specialist in writing DITA/AsciiDoc short descriptions. I map document structures, evaluate existing abstracts for quality, and ensure all abstracts provide immediate user value. |
| ## 2. Files to exclude from abstract auditing | ||
| The following file types do not require abstracts — skip them entirely: | ||
| - Assembly files: `assembly_*.adoc` — in this repository, assemblies do not carry their own abstract. The introduction and short description live in the first included concept module. | ||
| - Attribute files: `attributes*.adoc`, `_title-attributes.adoc` |
There was a problem hiding this comment.
| - Attribute files: `attributes*.adoc`, `_title-attributes.adoc` | |
| - Attribute files: `attributes*.adoc` |
| | Title | Preferred Short Description | | ||
| | :--- | :--- | | ||
| | Creating a group and adding a system | Group many systems together in the Edge Management application to manage them more easily. For example, you can more easily mitigate vulnerabilities and update systems that are alike. | | ||
| | Browsing hosts in Satellite Web UI | Find and categorize your hosts within {Project} to get a quick overview of your managed infrastructure. Browsing hosts helps you understand your environment and identify specific host types for targeted actions. | |
There was a problem hiding this comment.
Please include an example with attributes or avoid branded terms.
Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
pr-processor
bot
added
Needs re-review
and removed
Waiting on contributor
4 participants
What changes are you introducing?
Adding three Claude skills to help with CQA work, particularly short descriptions.
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
Requested after a demo I did showcasing these – for use for the team's work with short descriptions.
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
x
Contributor checklists
Please cherry-pick my commits into: