-
-
Notifications
You must be signed in to change notification settings - Fork 318
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
docs: update docsgen scripting and add new docs content #6134
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
Co-authored-by: Cayman <[email protected]>
Co-authored-by: Cayman <[email protected]>
Co-authored-by: Cayman <[email protected]>
Co-authored-by: Cayman <[email protected]>
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.
Related to sim testing it seems good. 👍🏼
# Install from NPM [not recommended] | ||
|
||
<!-- prettier-ignore-start --> | ||
!!! danger |
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.
Might wanna keep this as a warning so people don't install lodestar via npm
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.
Updated.
Im curious if we should not publish the package anymore to prevent against this.
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.
Could be installed as a package in another project in which case it would have a lock file again. I don't know if anybody is doing that though.
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.
There are 793 install / week listed on NPM. Curious if that is all us and CI?
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.
Curious if that is all us and CI?
I have never installed the package, I don't see us doing that in the CI either. Not sure who installs that to be honest, the README of the package is also a bit out of date.
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.
I read in a comment in our code in depgraph.md
.
I did a search on github and it seems the only references to @chainsafe/lodestar
as a dependency are only very old forks.
Since its a security risk, perhaps we should stop publishing it and make it hidden for now and see who squeals that its gone. What do you think @dapplion and @wemeetagain?
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.
I'd like to solve via #3596
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6134 +/- ##
=============================================
+ Coverage 0 80.02% +80.02%
=============================================
Files 0 19 +19
Lines 0 1717 +1717
Branches 0 155 +155
=============================================
+ Hits 0 1374 +1374
- Misses 0 341 +341
- Partials 0 2 +2 |
Merging this now for inclusion in 1.13, we can continue to address doc improvements in future PRs. |
🎉 This PR is included in v1.13.0 🎉 |
Motivation
Adds to, and updates, existing docs site. Part of #5961 resolution. More to be added but this PR is getting pretty big as-is.
Description
Includes a bunch of new documentation content and also updates how the auto-generated documentation is rendered from the cli package.
Steps to start docs site locally
yarn build yarn build:docs cd docs pip install --user -r requirements.txt mkdocs serve -w pages
Navigate via browser to
http://127.0.0.1:8000/lodestar/