-
Notifications
You must be signed in to change notification settings - Fork 2
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
More category fixes #2
Changes from all commits
40e0b48
2dc8c40
1b5b69e
cf8641e
07ddb11
81981a9
cac2263
bf46601
fe04235
1d83621
e57ef22
38c8b80
7160f9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Release type: minor | ||
|
||
Added subcategories to categories to be able to cycle through which subcategories are present for a category. | ||
Updated plugin to better work with modern pelican standards (by updating tasks.py and ensuring proper code formatting) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,25 @@ | ||
import os | ||
from pathlib import Path | ||
from shutil import which | ||
|
||
from invoke import task | ||
|
||
PKG_NAME = 'more_categories' | ||
PKG_PATH = Path(f'pelican/plugins/{PKG_NAME}') | ||
FLAKE8 = 'flake8' | ||
ISORT = 'isort' | ||
PYTEST = 'pytest' | ||
|
||
ACTIVE_VENV = os.environ.get("VIRTUAL_ENV", None) | ||
VENV_HOME = Path(os.environ.get("WORKON_HOME", "~/.local/share/virtualenvs")) | ||
VENV_PATH = Path(ACTIVE_VENV) if ACTIVE_VENV else (VENV_HOME / PKG_NAME) | ||
VENV = str(VENV_PATH.expanduser()) | ||
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer activating a suitable virtual environment manually. In particular because I (and maybe others) don't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just following the standard of all the other plugins and pelican itself. All the other ones are using virtual env and in order to keep everything consistent it would make sense to replicate those no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, not if it disrupts the preferred tooling of plugin authors and others who prefer alternate workflows. 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. Sorry .-. I'll go ahead and undo this as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So for this one. If I changed it so that it used the same thing like poetry:
would that be ok @oulenz ? It would be the ebst of both worlds right? It uses paths when they exist, else it looks inside the environment bin. Or would you prefer I just use the same method that you had before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would work. I have to admit, I am getting utterly confused by how all these tools inter-operate. In this case, what I don't understand is why we'd want to use invoke to call poetry, rather than the other way around. If you do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll go ahead and do that for now =) Maybe @justinmayer would have more idea why we use invoke for something like this? I'm still a newbie... .-. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My task automation priorities have been to (1) be flexible and accommodate different workflows and (2) make it easy for folks to set up a Pelican development environment. Regarding the first priority, there are (as we've discussed) many ways to handle virtual environments. I prefer to manage mine manually, rather than let Poetry manage them for me. At the same time, people should also be free to let Poetry handle virtual environments on their behalf. Along similar lines, some people (like me) prefer having Poetry and Pre-commit installed globally instead of having them only live in virtual environments. Some folks don't. Regarding the second objective, the reason I use Invoke to call Poetry here is to keep things simple for new contributors. By putting as much as possible into Invoke tasks, that means that installing Invoke becomes the primary pre-requisite, and then Invoke can handle most of the rest. So in theory, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation, it's helpful to understand what is logically necessary and what are design decisions. I agree that focusing everything on invoke and |
||
|
||
|
||
TOOLS = ['poetry', 'pre-commit'] | ||
FLAKE8 = which('flake8') or Path(f'{VENV}/bin/flake8') | ||
ISORT = which('isort') or Path(f'{VENV}/bin/isort') | ||
PYTEST = which('pytest') or Path(f'{VENV}/bin/pytest') | ||
PIP = which('pip') or Path(f'{VENV}/bin/pip') | ||
POETRY = which('poetry') or Path(f'{VENV}/bin/poetry') | ||
PRECOMMIT = which('pre-commit') or Path(f'{VENV}/bin/pre-commit') | ||
|
||
|
||
@task | ||
|
@@ -32,3 +45,25 @@ def flake8(c): | |
def lint(c): | ||
isort(c, check=True) | ||
flake8(c) | ||
|
||
|
||
@task | ||
def tools(c): | ||
"""Install tools in the virtual environment if not already on PATH""" | ||
for tool in TOOLS: | ||
if not which(tool): | ||
c.run(f'{PIP} install {tool}') | ||
|
||
|
||
@task | ||
def precommit(c): | ||
"""Install pre-commit hooks to .git/hooks/pre-commit""" | ||
c.run(f'{PRECOMMIT} install') | ||
|
||
|
||
@task | ||
def setup(c): | ||
c.run(f'{PIP} install -U pip') | ||
tools(c) | ||
c.run(f'{POETRY} install') | ||
precommit(c) |
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 should probably add a changelog, but I don't understand this addition. Is it it meant to be converted to a changelog by some automation tool?
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 followed the guide on Pelican's website. See: https://docs.getpelican.com/en/latest/contribute.html#submitting-your-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.
That's my fault, Oliver. This bit is related to AutoPub, a tool that I built to automate GitHub + PyPI releases upon pull request merge. I had meant to add AutoPub integration to More Categories as well, but that to-do item seems to have slipped between the cracks. I will submit a separate pull request for that for you to review, which will include continuous integration (CI) for More Categories, so its tests will automatically be run on pull requests such as this one. 😄
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.
So should I leave this one as is for now?