-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add theme #85
Add theme #85
Conversation
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.
Missing the env file changes?
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.
Couple of questions, otherwise lgtm! Thanks!
{{cookiecutter.repo_name}}/docs/source/_static/logo/placeholder_favicon.svg
Show resolved
Hide resolved
Looking through the tests - do any of them actually test builds without MDAnalysis as the base org? I can't seem to find the full matrix anywhere. |
@IAlibay the tests are Python tests, not options in CI -- the matrix is here: cookiecutter-mdakit/tests/test_output.py Lines 33 to 35 in 2bb32a3
I initially verified the options by running the cookiecutter myself but I've added them in as tests now, which was good since the github options actually uncovered another bug. I'll leave this open for another day or two in case you wanted to look at and comment on it -- I know you're busy, though, so no need to re-review if you don't have the time. If you did want to re-review, I pushed updates from this PR to |
Thanks, I'll have a look, might not get to it until tomorrow if it can wait - co-running a lot of NF sessions today. |
Thanks, yeah no rush at all. On Sep 11, 2023, at 22:28, Irfan Alibay ***@***.***> wrote:
Thanks, I'll have a look, might not get to it until tomorrow if it can wait - co-running a lot of NF sessions today.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
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.
Sorry about the slow review! Overall LGTM, there's just a couple of small questions but neither of which I consider to be necessary and/or blocking. (mostly curiosities)
|
||
|
||
# -- General configuration --------------------------------------------------- | ||
|
||
# If your documentation needs a minimal Sphinx version, state it here. | ||
# | ||
# needs_sphinx = '1.0' | ||
# needs_sphinx = "1.0" |
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.
The theme requires a minimum of 6.2.1, do we want to set this 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.
Sure, I've set it.
Hello @lilyminium! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-09-21 09:03:23 UTC |
Fixes #84
Changes made in this Pull Request:
PR Checklist