Skip to content
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

Closed
9 changes: 8 additions & 1 deletion README.md
Expand Up @@ -47,6 +47,13 @@ case this plugin in present, use:
Likewise, `category.shortname`, `category.parent` and `category.ancestors` can
also be used on the category template.

Additionally, this plugin adds `category.subcategories`: a `list` of categories
that have `category` as a parent.

{% for subcat in category.subcategories %}
<a href="{{ SITEURL }}/{{subcat.url}}">{{subcat.shortname|capitalize}}</a>
{% endfor %}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"category.children and category.descendents", and change the example to use category.children

### Slug
The slug of a category is generated recursively by slugifying the shortname of
the category and its ancestors, and preserving slashes:
Expand All @@ -73,4 +80,4 @@ categories always follow their parent:

aba
aba/dat
abaala
abaala
4 changes: 4 additions & 0 deletions RELEASE.md
@@ -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)
Comment on lines +1 to +4
Copy link
Collaborator

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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. 😄

Copy link
Author

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?

21 changes: 19 additions & 2 deletions pelican/plugins/more_categories/more_categories.py
Expand Up @@ -3,7 +3,7 @@
Title: More Categories
Description: adds support for multiple categories per article and nested
categories
Requirements: Pelican 3.8 or higher
Requirements: Pelican 4.0.0 or higher
"""

from collections import defaultdict
Expand Down Expand Up @@ -43,7 +43,6 @@ def slug(self):
else:
subs = self.settings.get('SLUG_REGEX_SUBSTITUTIONS', [])
self._slug = slugify(self.shortname, regex_subs=subs)
print(self._slug)
thecaligarmo marked this conversation as resolved.
Show resolved Hide resolved
if self.parent:
self._slug = self.parent.slug + '/' + self._slug
return self._slug
Expand Down Expand Up @@ -79,6 +78,24 @@ def create_categories(generator):
)
generator._update_context(['categories'])

# Add subcategories and children
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to children and descendents

descendents = defaultdict(list)
children = defaultdict(list)
for category, articles in generator.categories:
for anc in category.ancestors[1:]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this should be category.ancestors[:-1]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed =)

Also, for the "rearrange all commits into three commits" part. To be honest, I'm not sure how to do that .-. Could you do that? (Or tell me what I should be doing and I can do it)

descendents[anc].append(category)
if anc == category.parent:
children[anc].append(category)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly inelegant, could you place it outside the inner loop, and change it to:

if category.parent:
    children[category.parent].append(category)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much better. Good idea =D

for category, articles in generator.categories:
if category.slug in descendents:
category.descendents = descendents[category]
else:
category.descendents = []
if category.slug in children:
category.children = children[category]
else:
category.children = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're using defaultdicts, we can simply do:

category.descendents = sorted(descendents[category])
category.children = sorted(children[category])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not used to defaultdicts .-. I really need to get used to them though cause that's so much nicer. =D



def register():
signals.article_generator_context.connect(get_categories)
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Expand Up @@ -46,7 +46,7 @@ invoke = "^1.3"
isort = "^4.3"
livereload = "^2.6"
markdown = "^3.1.1"
pytest = "^5.0"
pytest = ">5.0,!=5.4.0,!=5.4.1"
pytest-cov = "^2.7"
pytest-pythonpath = "^0.7.3"
pytest-sugar = "^0.9.2"
Expand Down
41 changes: 38 additions & 3 deletions tasks.py
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 virtualenv but conda.

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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. 😉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Sorry .-. I'll go ahead and undo this as well.

Copy link
Author

Choose a reason for hiding this comment

The 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:

POETRY = which("poetry") if which("poetry") else (VENV / Path("bin") / "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?

Copy link
Collaborator

@oulenz oulenz Apr 23, 2020

Choose a reason for hiding this comment

The 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 poetry run invoke ..., doesn't poetry then handle the environment, so we could have a cleaner tasks.py file? 🤔

Copy link
Author

Choose a reason for hiding this comment

The 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... .-.

Copy link
Contributor

Choose a reason for hiding this comment

The 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, pip install invoke && invoke setup becomes all that's required to get going. The concept of letting Poetry manage the environment and then using poetry run invoke … is valid enough, but that would depend on Poetry being globally installed, and I didn't want to force users to globally install anything just in order to contribute to the project. The way I configured tasks.py and the relevant instructions is that it all works with a minimal number of steps and regardless of whether Poetry and Pre-commit are globally installed or not.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 tasks.py is probably the easiest to understand for new contributors.



TOOLS = ['poetry', 'pre-commit']
FLAKE8 = which('flake8') if which('flake8') else (VENV / Path('bin') / 'flake8')
ISORT = which('isort') if which('isort') else (VENV / Path('bin') / 'isort')
PYTEST = which('pytest') if which('pytest') else (VENV / Path('bin') / 'pytest')
PIP = which('pip') if which('pip') else (VENV / Path('bin') / 'pip')
POETRY = which('poetry') if which('poetry') else (VENV / Path('bin') / 'poetry')
PRECOMMIT = which('pre-commit') if which('pre-commit') else (VENV / Path('bin') / 'pre-commit')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these ternary statements can all be simplified to:

FLAKE8 = which('flake8') or Path(f'{VENV}/bin/flake8')

Also, TOOLS should come at the end, and say

TOOLS = [POETRY, PRECOMMIT]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change TOOLS to what you have, won't that break the loop that iterates over TOOLS in tools()? It feels counter-productive to do TOOLS = [POETRY, PRECOMMIT] since the point of TOOLS is to install poetry and pre-commit if they are not already installed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! So if PIP is set to Path(f'{VENV}/bin/pip'), and we run f'{PIP} install {tool}', will the tool be automatically installed in the virtual_env?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, it should.



@task
Expand All @@ -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)