-
Notifications
You must be signed in to change notification settings - Fork 1
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
DX: workflows including MyST NB and precommit added #9
Conversation
for more information, see https://pre-commit.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.
Looks good already! Just a few changes
Improved :) Ready to go~ |
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.
Thanks, almost there! 👏
pixi.toml
Outdated
authors = ["Vitor Shen <[email protected]>"] | ||
channels = ["conda-forge"] | ||
description = "Add a short description here" |
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 think it's fine to leave out version
and description
for now. They're optional
https://pixi.sh/latest/reference/configuration/#version-optional
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.
Last change 🙈
.github/workflows/deploy.yml
Outdated
- uses: actions/setup-node@v4 | ||
uses: actions/configure-pages@v5 | ||
with: | ||
node-version: 18.x | ||
- name: Install MyST Markdown | ||
run: npm install -g mystmd | ||
- name: Build HTML Assets | ||
run: myst build --html | ||
- name: Upload artifact | ||
uses: actions/upload-pages-artifact@v1 | ||
enablement: "true" |
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 think it's not needed. Never used it for any other ComPWA project and those work just fine.
For sure it's not needed in the build
step 😅
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.
But seems like this is working fine too. I think I'll keep it for https://pages.github.com/
.github/workflows/deploy.yml
Outdated
id: deployment | ||
uses: actions/deploy-pages@v2 | ||
path: ./_build/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.
Building the pages with pixi run doc
is still missing here. That's also what the with.path
was originally for (not setup-pixi
, that requires no arguments).
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.
Actually https://github.com/actions/upload-pages-artifact is also missing. That is where with.path
should be used.
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.
ok this is now added
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.
Ok. Done
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.
Just noticed some bugs in the workflow
.github/workflows/deploy.yml
Outdated
id: deployment | ||
uses: actions/deploy-pages@v2 | ||
path: ./_build/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.
Actually https://github.com/actions/upload-pages-artifact is also missing. That is where with.path
should be used.
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.
Awesome 🎉
Closes #7