-
Notifications
You must be signed in to change notification settings - Fork 34
chore: added markdown templates to docs #237
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
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Markdown documentation generation system is introduced for Solidity contracts. It includes a configuration module, Handlebars templates for rendering contract pages, and helper modules for content processing and AST analysis. The hardhat configuration is updated to reference the new documentation system. Changes
Sequence DiagramsequenceDiagram
participant Hardhat
participant Config as config-md.js
participant Templates as Templates (contract.hbs)
participant Helpers as helpers.js
participant AST as properties.js
Hardhat->>Config: Load documentation config
Config-->>Hardhat: Return output dir, templates dir, pages resolver
Hardhat->>Templates: Render contracts with template
Templates->>Helpers: Process NATSPEC content
Helpers->>Helpers: Cache cross-references (getAllLinks)
Helpers->>Helpers: Replace reference patterns (processReferences)
Helpers->>Helpers: Convert callouts and admonitions (processCallouts)
Helpers-->>Templates: Return processed content
Templates->>AST: Extract item properties
AST->>AST: Analyze inheritance chain
AST->>AST: Generate anchors from signatures
AST-->>Templates: Return properties for rendering
Templates-->>Hardhat: Generate markdown pages with YAML front matter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 5
🧹 Nitpick comments (6)
docs/templates-md/properties.js (2)
30-32: Clarify the trailing dash removal logic.The code removes a trailing dash if present, but it's unclear when this edge case would occur. The slug function (from helpers.js) replaces non-word characters with dashes, so an empty signature
()would become---, which could explain this check.Consider adding a comment to clarify when this edge case occurs:
if (res.charAt(res.length - 1) === '-') { + // Remove trailing dash that can occur with empty signatures return res.slice(0, -1); }
36-45: Consider adding a comment for Context filtering logic.The inheritance function has specific logic to filter out 'Context' except when it's the first item (i === 0). This business logic would benefit from a comment explaining why Context requires special handling.
module.exports.inheritance = function ({ item, build }) { if (!isNodeType('ContractDefinition', item)) { // Return empty array for non-contracts (interfaces, libraries) return []; } return item.linearizedBaseContracts .map(id => build.deref('ContractDefinition', id)) + // Context is a base contract that should be hidden from docs unless it's the main contract .filter((c, i) => c.name !== 'Context' || i === 0); };docs/templates-md/helpers.js (4)
31-42: Consider concurrency safety if templates are rendered in parallel.The global
functionNameCountsobject works for single-threaded documentation generation but could cause race conditions if templates are ever rendered concurrently.
67-72: Enhance input validation.The slug function only checks for
undefinedbut doesn't handlenullor other invalid types. Consider using a more robust check.const slug = (module.exports.slug = str => { - if (str === undefined) { + if (typeof str !== 'string' || !str) { throw new Error('Missing argument'); } return str.replace(/\W/g, '-'); });
320-323: Add timeout to prevent hanging builds.The
execSynccall lacks a timeout, which could cause the documentation build to hang indefinitely ifdowndocencounters issues.execSync(`npx downdoc "${tempAdocFile}"`, { stdio: 'pipe', cwd: process.cwd(), + timeout: 30000, // 30 second timeout + maxBuffer: 10 * 1024 * 1024, // 10MB buffer });
353-411: Consider refactoring for maintainability.The
processCalloutsfunction performs 15+ sequential regex replacements, making it difficult to maintain and potentially impacting performance on large documents. Consider:
- Grouping related patterns into helper functions
- Using a single pass with a more sophisticated regex where possible
- Adding inline comments explaining each transformation's purpose
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/config-md.js(1 hunks)docs/templates-md/contract.hbs(1 hunks)docs/templates-md/helpers.js(1 hunks)docs/templates-md/page.hbs(1 hunks)docs/templates-md/properties.js(1 hunks)hardhat.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
hardhat.config.ts (2)
docs/templates-md/helpers.js (2)
require(1-1)require(4-4)docs/templates-md/properties.js (2)
require(1-1)require(2-2)
docs/templates-md/properties.js (1)
docs/templates-md/helpers.js (2)
slug(67-72)i(139-139)
docs/config-md.js (1)
docs/templates-md/helpers.js (4)
path(3-3)require(1-1)require(4-4)fs(2-2)
docs/templates-md/helpers.js (1)
docs/templates-md/properties.js (2)
require(1-1)require(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: tests
🔇 Additional comments (14)
hardhat.config.ts (1)
37-37: LGTM!The configuration update correctly points to the new markdown documentation config file.
docs/templates-md/page.hbs (1)
1-13: LGTM!The page template structure is clean and follows Handlebars conventions. The YAML front matter, conditional prelude, and items iteration logic are all properly implemented.
docs/templates-md/contract.hbs (2)
3-3: Verify the inline style syntax.The inline style uses
style=\{{...}}syntax with escaped braces. Please verify this renders correctly in the target MDX environment and that the backslash escaping is intentional for JSX compatibility.
14-14: Verify the import path prefix.The import statement uses
@openzeppelin/prefix followed by the absolute path. Ensure this matches the actual package structure foropenzeppelin-confidential-contracts. The standard OpenZeppelin contracts use@openzeppelin/contracts/, but this repository may require a different prefix.docs/templates-md/properties.js (1)
82-91: LGTM!The
inherited-functionslogic correctly filters out base functions and handles constructor visibility. The implementation properly groups functions by their declaring contract.docs/templates-md/helpers.js (9)
1-9: LGTM!The imports and version export follow standard Node.js patterns. The use of built-in modules is appropriate for a documentation generation helper.
17-27: LGTM - Appropriate error handling for optional content.The pattern of logging warnings and returning empty strings is appropriate for optional README content in documentation generation. The existence check prevents unnecessary errors.
74-124: LGTM - Effective caching strategy.The two-level caching approach using WeakMap and Map is appropriate for this use case, allowing garbage collection while maintaining per-page caches. The minor duplication in cache setup (lines 80-84 vs 115-119) is acceptable for code clarity.
126-158: LGTM!The relative path calculation logic correctly handles same-page anchors, sibling pages, and pages at different directory levels. The approach of finding the common base and calculating up/down navigation is sound.
284-299: LGTM!The HTML entity replacements are in the correct order (with
&processed last to avoid double-decoding), and the URL markdown conversion pattern is appropriate.
225-249: LGTM!The reference resolution logic appropriately tries exact matches before falling back to fuzzy matching. The two-level approach balances precision with flexibility for cross-references.
413-430: LGTM!The title and description helpers provide reasonable default formatting for page metadata derived from the file path structure.
432-439: LGTM!The
with-preludehelper follows the same processing pattern asprocess-natspec, maintaining consistency in how content is transformed throughout the documentation generation pipeline.
52-61: LGTM!The
process-natspecfunction provides a clean orchestration layer, delegating to specialized functions for link generation, reference processing, and callout handling.
|
Please follow the example of the community contracts PR and make a new one internally |
This PR adds the
templates-mdandconfig-md.jsto thedocsdirectory, allowing us to generate API docs remotely inOpenZeppelin/docs. Please see the guide here.Summary by CodeRabbit