-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
List instead of table #327
Conversation
e152fbd
to
6e64aaa
Compare
6e64aaa
to
487155c
Compare
@@ -17,6 +17,8 @@ jobs: | |||
ruby-version: '3.3' | |||
env: | |||
BUNDLE_WITHOUT: release | |||
- name: Generate database | |||
run: bundle exec rake database |
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.
should we update it on each release or on each merge?
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.
mhm what does that actually do? just build a yard db that will be added to the gem before it's uploaded?
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.
It builds the old table from the README and several lists sorted in different ways. @ekohl had added it to the .gitignore
in PR #262, so I kept it that way, meaning this would be run on release. Does it make sense to run on every merge instead and remove from the .gitignore
? Run on every merge and just update GH pages?
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.
You can see the output from YARD here: https://www.cs.umd.edu/~ykatz/facterdb/file.list_arch_os_facter.html
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'm a big fan of not including generated files in git, so will generally approve PRs that avoid it. That said, if this job doesn't show it then I assume it's verify the job passes. Is that correct?
432ac5e
to
0b44356
Compare
I think this is fine for now and we should merge it so it lands in the next major release. We can always improve it afterwards. @yakatz can you rebase and merge afterwards? |
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.
Not a complete review, just some things that stood out to me.
.github/workflows/pages.yml
Outdated
path: doc | ||
- name: Deploy to GitHub Pages | ||
id: deployment | ||
uses: actions/deploy-pages@v1 |
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.
Latest version is v4
uses: actions/deploy-pages@v1 | |
uses: actions/deploy-pages@v4 |
.github/workflows/pages.yml
Outdated
- name: Set up Pages | ||
uses: actions/configure-pages@v2 | ||
- name: Upload artifact | ||
uses: actions/upload-pages-artifact@v1 |
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.
Latest is v3
uses: actions/upload-pages-artifact@v1 | |
uses: actions/upload-pages-artifact@v3 |
.github/workflows/pages.yml
Outdated
- name: Build docs | ||
run: bundle exec rake yard | ||
- name: Set up Pages | ||
uses: actions/configure-pages@v2 |
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.
v5 is latest
uses: actions/configure-pages@v2 | |
uses: actions/configure-pages@v5 |
@@ -17,6 +17,8 @@ jobs: | |||
ruby-version: '3.3' | |||
env: | |||
BUNDLE_WITHOUT: release | |||
- name: Generate database | |||
run: bundle exec rake database |
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'm a big fan of not including generated files in git, so will generally approve PRs that avoid it. That said, if this job doesn't show it then I assume it's verify the job passes. Is that correct?
This changes the workflow to have a rake task to create a database.md file and then render that into the generated documentation. The release process is also enhanced to include the generated database.md file. This should allow sites like rubydoc.info correctly render it as well, though this hasn't been tested. It also introduces a GitHub Action workflow to build GitHub Pages with the generated documentation. That means that the documentation should always be up to date and no manual steps are needed anymore. It also saves a lot of noise in the git log.
Based on voxpupuli#262
0b44356
to
aea3ffa
Compare
Based on (includes) #262, adds additional ways of seeing the included facts