-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Docs: introduce product identifier #21670
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
Conversation
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.
Hey @naltatis - I've reviewed your changes - here's some feedback:
- Instead of relying on os.Stat on each filename, aggregate and validate all product.Identifier() values upfront to catch duplicates in memory and provide clearer error messages.
- Avoid mutating slug.CustomSub as a global; create and configure a local slug instance (or reset the global) to prevent unintended side‐effects during identifier generation.
- Wrap the "params:" section in a conditional in documentation.tpl to skip emitting an empty header when no non-deprecated params are present.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| var modbusRender string | ||
| modbusData := make(map[string]interface{}) |
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.
Warum brauchts das?
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.
Plan ist auf der Produkt-Seite in der Doku die einzelnen Parameter tabellarisch zu erklären. Bei den Modbus-Geräten haben wir ja implizite Parameter erzeugt, die durch die Modbus Eigenschaft erzeugt werden. Dafür braucht es die Modbus Infos (Defaults, ...) des Geräts. Quasi das Äquivalent zur documentation_modbus.tpl.
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.
Die werden doch aber- falls modbus- weiter unten eh angelegt?
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.
Was meinst du damit? Modbus ist ja immer eine Sonderbehandlung in der Darstellung. Unterschiedliche Optionen/Spielarten aus denen der Nutzer auswählen muss und so.
Wenns um die konkrete Zeile geht. Die hab ich nur weiter nach vorne geschoben, damit die Daten nicht nur in documentation_modbus.tpl verwendet werden können, sondern auch als strukturierte Daten mit in die Doku-Yaml Datei aufgenommen werden.
Add unique product identifier based on brand and description. Basis for upcoming product detail pages in docs evcc-io/docs#814 and icon referencing from https://github.com/evcc-io/icons.
🏷️ introduce product identifier
"Skoda iV Charger Connect+" > skoda-iv-charger-connectplus👯 test product name uniqueness, fix collisions (OCPP dups) \cc @premultiply
⚙️ use identifier for generated doc filenames (instead of index)
🔗 add template name for back reference (edit this page)
🍱 add params & modbus data structure to show as table on docs detail pages
Note: Product identifiers are not stable since product names are adjusted from time to time. We might replace this with a more permanent solution or introduce a compatibility feature (like
coversin templates) in the future. For now this is a pragmatic approach.relates to evcc-io/docs#814