-
Notifications
You must be signed in to change notification settings - Fork 16
Removes docs directory #263
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
base: main
Are you sure you want to change the base?
Conversation
andrew-fleming
left a 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.
Looks good! We still have the old site that's connected: https://old-docs.openzeppelin.com/contracts-compact/0.0.1/ . I think we should create a docs branch then update the playbook on the old docs repo here like this:
- url: https://github.com/OpenZeppelin/compact-contracts
branches:
- docs-v*
start_path: docs
so it can fetch the last prerelease (as opposed to reading from HEAD as it is now). WDYT?
Also, I think we can remove the docs tasks in turbo.json. I just left a question and a small suggestion. Lastly, I'd create an issue since it's not a trivial change so we can track it in the project :)
| The online version of the Compact contracts documentation is maintained at [OpenZeppelin/docs](https://github.com/OpenZeppelin/docs). | ||
| It should mirror all documentation embedded within the contracts themselves with additional context and usage examples if applicable. | ||
| Pull requests in [OpenZeppelin/docs](https://github.com/OpenZeppelin/docs) should be linked to the corresponding pull request in this repo. | ||
|
|
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.
Sooooooooooooo this adds a ton of friction for contributors. It's a good idea to have some coordination with linking a PR from the docs repo as you mentioned; however, this feels messy. The consequences of centralization :(
I was considering if we could place the docs repo in a submodule or subtree so all changes are, from the contributor's perspective, from the same project. The former is a bit more convenient for the contributor but they need to know how to work with submodules. The latter adds complexity to us but improves dx for the contributor...or keep it as-is. Thoughts?
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.
Totally agree, I think we can make this easier on contributors by using Git subtrees. I can add some scripts and more info to CONTRIBUTING.md to standardize the workflow.
package.json
Outdated
| "scripts": { | ||
| "docs": "turbo run docs --filter=docs", | ||
| "docs:watch": "turbo run docs:watch --filter=docs", | ||
| "compact": "turbo run compact --filter=@openzeppelin-compact/contracts --log-prefix=none", | ||
| "build": "turbo run build --filter=!'docs' --log-prefix=none", |
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.
We can remove --filter=!'docs' in build
|
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 WalkthroughThe pull request deprecates the embedded documentation directory by removing all module documentation pages, API references, Antora configuration, and documentation build scripts. Documentation content is being migrated to an external repository, and references in root-level configuration files are updated accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 0
🧹 Nitpick comments (1)
GUIDELINES.md (1)
30-32: Add comma for clarity.Line 31 would read more clearly with a comma before the prepositional phrase.
Apply this diff:
- It should mirror all documentation embedded within the contracts themselves with additional context and usage examples if applicable. + It should mirror all documentation embedded within the contracts themselves, with additional context and usage examples if applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)CONTRIBUTING.md(1 hunks)GUIDELINES.md(1 hunks)docs/antora.yml(0 hunks)docs/modules/ROOT/nav.adoc(0 hunks)docs/modules/ROOT/pages/access.adoc(0 hunks)docs/modules/ROOT/pages/api/access.adoc(0 hunks)docs/modules/ROOT/pages/api/fungibleToken.adoc(0 hunks)docs/modules/ROOT/pages/api/multitoken.adoc(0 hunks)docs/modules/ROOT/pages/api/nonFungibleToken.adoc(0 hunks)docs/modules/ROOT/pages/api/security.adoc(0 hunks)docs/modules/ROOT/pages/api/utils.adoc(0 hunks)docs/modules/ROOT/pages/extensibility.adoc(0 hunks)docs/modules/ROOT/pages/fungibleToken.adoc(0 hunks)docs/modules/ROOT/pages/index.adoc(0 hunks)docs/modules/ROOT/pages/multitoken.adoc(0 hunks)docs/modules/ROOT/pages/nonFungibleToken.adoc(0 hunks)docs/modules/ROOT/pages/security.adoc(0 hunks)docs/modules/ROOT/pages/utils.adoc(0 hunks)docs/modules/ROOT/pages/zkCircuits101.adoc(0 hunks)docs/package.json(0 hunks)package.json(0 hunks)
💤 Files with no reviewable changes (19)
- docs/modules/ROOT/nav.adoc
- docs/modules/ROOT/pages/access.adoc
- docs/modules/ROOT/pages/multitoken.adoc
- docs/modules/ROOT/pages/api/nonFungibleToken.adoc
- docs/modules/ROOT/pages/security.adoc
- docs/modules/ROOT/pages/index.adoc
- docs/modules/ROOT/pages/api/security.adoc
- docs/modules/ROOT/pages/utils.adoc
- docs/antora.yml
- package.json
- docs/modules/ROOT/pages/zkCircuits101.adoc
- docs/modules/ROOT/pages/api/access.adoc
- docs/package.json
- docs/modules/ROOT/pages/fungibleToken.adoc
- docs/modules/ROOT/pages/extensibility.adoc
- docs/modules/ROOT/pages/nonFungibleToken.adoc
- docs/modules/ROOT/pages/api/utils.adoc
- docs/modules/ROOT/pages/api/fungibleToken.adoc
- docs/modules/ROOT/pages/api/multitoken.adoc
🧰 Additional context used
🪛 LanguageTool
GUIDELINES.md
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...mentation embedded within the contracts themselves with additional context and usage examp...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ 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). (2)
- GitHub Check: Run Test Suite
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
CONTRIBUTING.md (1)
91-91: LGTM.Minor formatting correction improves consistency.
.github/PULL_REQUEST_TEMPLATE.md (1)
27-29: LGTM.PR checklist links are correctly updated to reference the new guidance sections in GUIDELINES.md. All anchor links are valid and will guide contributors appropriately.
andrew-fleming
left a 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.
Hey @emnul I see we have the docs branch 👍
we just need to remove the --filter=!'docs', right?
|
Currently blocked by OpenZeppelin/docs.openzeppelin.com#458 |
| function processFile(filePath) { | ||
| try { | ||
| const content = fs.readFileSync(filePath, "utf8"); | ||
| const updatedContent = content.replace(/```rust/g, "```rust"); |
Check warning
Code scanning / CodeQL
Replacement of a substring with itself Medium documentation
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the issue, determine the intended transformation for the string "rust". If the intention is to escape quotes, convert code fences from "rust" to another language (e.g., "```cairo"), or apply any other specific transformation, the replacement string should reflect that. Since the script is named "cairo-convert", it is very likely meant to convert Rust code fences to Cairo code fences in the docs:
- General fix: Change the replacement string from "
rust" to the intended value, such as "cairo", so that all "rust" code blocks in Markdown are converted to "cairo" blocks. - Detailed steps:
- Edit docs/scripts/cairo-convert.js at line 22.
- Change the replacement code to:
const updatedContent = content.replace(/```rust/g, "```cairo");
- No new imports or definitions are required; just the single line edit.
-
Copy modified line R22
| @@ -19,7 +19,7 @@ | ||
| function processFile(filePath) { | ||
| try { | ||
| const content = fs.readFileSync(filePath, "utf8"); | ||
| const updatedContent = content.replace(/```rust/g, "```rust"); | ||
| const updatedContent = content.replace(/```rust/g, "```cairo"); | ||
|
|
||
| if (content !== updatedContent) { | ||
| fs.writeFileSync(filePath, updatedContent, "utf8"); |
7805029 to
26866a7
Compare
git-subtree-dir: docs git-subtree-split: f1cf40f52fe7be77fbc603b90051262c78990556
andrew-fleming
left a 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.
Yikes, this is major major bloat with the docs repo in here. AFAICT there's no how-to in the guidelines here. The idea is to open a PR in this repo with changes to the docs and then we directly push the subtree changes (git subtree push) into the main branch of the docs? If so, I think this should be further explored and discussed outside of this PR especially with the docs team. Forgive me, I know I brought up the idea to ease friction. LMK if I'm missing something or if you disagree
Yup, the idea is essentially to colocate the docs repo in this repo so users can directly modify the external repo code without having to go back and forth between two different repos. Using subtrees also makes it easier for beginners to understand the workflow. It definitely adds bloat which is one of the major drawbacks of subtrees, alternatively we could use submodules. For our use case, we only care about updating |
|
@andrew-fleming what do you think about the following flow as suggested by @stevedylandev
Users can view if the build fails / everything looks right in the |
Removes docs directory since we're now maintaining our online docs at OpenZepplin/docs. Updates GUIDELINES and CONTRIBUTING files
Closes #265
Summary by CodeRabbit
Documentation
Chores