More category fixes #2

thecaligarmo · 2020-04-20T00:38:33Z

This fix does two things:

Adds category.subcategores which is a list of categories which contain category as a parent.
Updates plugin to use the latest code testing version for pelican-plugins.

…o force version 5.3.5

oulenz · 2020-04-20T16:17:21Z

Thanks for your contribution!

I think it's best if we think through the various use cases. I assume you need access to all the ancestors of a category, not just its children?

If so, I think we should seperately offer access to the children of a category.

thecaligarmo · 2020-04-20T16:48:36Z

There is already functionality to get the ancestors (parents) of a category. It did not have the functionality to get the children. So what I needed was access to the children.

What you're suggesting is exactly what I added. I separately added access to the children of a category.

oulenz · 2020-04-20T17:58:14Z

Sorry, I meant to say descendents. You are adding all the descendents, but I could image someone wanting to list only the children. (E.g. the way the content of categories is displayed on Wikipedia.)

oulenz

I've added some inline comments. I think this PR would be easier to handle if you just proposed the descendents/children functionality (and not all the code style changes).

README.md

oulenz · 2020-04-20T20:54:08Z

pelican/plugins/more_categories/more_categories.py

@@ -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.2.0 or higher


Why 4.2.0? (3.8 became 4.0, but that should be enough.)

That should say 4.0.0. Sorry. The reason I changed it is because in the README it said 4.0.0. From what I can tell, it's requiring 4.0 functionality, so I decided to change it. Was that wrong?

Once this plugin has been published to PyPI, if the goal is to be able to run pip install pelican-more-categories and have everything Just Work™, I believe the next release (tentatively planned as version 4.5) would be required, as the namespace plugin functionality was merged to master after v4.2 was released.

@thecaligarmo Changing that to 4.0.0 is fine, absolutely!

@justinmayer Yes, but, users with older versions of pelican could still use this plugin the old-fashioned way right? Also, any particular reason why you want to skip from 4.2 to 4.5, or are you planning intermediate releases?

@oulenz: Good point. I agree that specifying 4.0.0 makes more sense. Regarding the next release, current master drops Python 2.7 support and will most likely also drop Python 3.5 support. Plus, there are a decent number of changes in master since 4.2. As I mentioned in another thread, there aren't any changes I consider to be truly backwards-incompatible, so 5.0 seemed too big and an unwarranted jump, while 4.3 didn't seem to capture the extent of the changes, so I split the difference and suggested 4.5. Sometimes strict semantic versioning doesn't fit all situations. 🤷‍♂️

oulenz · 2020-04-20T20:55:54Z

pelican/plugins/more_categories/more_categories.py

@@ -19,13 +19,13 @@ class Category(URLWrapper):
    @property
    def _name(self):
        if self.parent:
-            return self.parent._name + '/' + self.shortname
+            return self.parent._name + "/" + self.shortname


Please don't change single quotes to double quotes.

This is done automatically using the tasks that are present within pelican. In the lint section they are using "black". Black makes it so that single quotes are turned to double quotes to make coding conventions the same across the board. Since black is included in every other plugin and in (latest) pelican, I assume that is what is wanted.

While I thought it would be beneficial to have consistent code style across the board, I understand that code style is very much subjective and something that shouldn't be forcibly foisted upon plugin developers who prefer a different style. So plugin authors are free to use their own conventions, as @oulenz has done with More Categories. For example, as you can see, Black is (purposely) not among the tools run during the lint task in this repository. So while I fully understand and appreciate your intention here, it would be best to honor this repository's style and not change these quotation marks. 😊

Ok cool. I'll go ahead and undo those then. Sorry about that @oulenz

pelican/plugins/more_categories/more_categories.py

pyproject.toml

oulenz · 2020-04-20T21:00:01Z

tasks.py

+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())


I prefer activating a suitable virtual environment manually. In particular because I (and maybe others) don't use virtualenv but conda.

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?

Well, not if it disrupts the preferred tooling of plugin authors and others who prefer alternate workflows. 😉

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

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?

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

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

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.

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.

oulenz · 2020-04-20T21:02:09Z

tasks.py

+def black(c, check=False, diff=False):
+    """Run Black auto-formatter, optionally with --check or --diff"""
+    check_flag, diff_flag = "", ""
    if check:
-        check_flag = '-c'
-    c.run(f'{ISORT} {check_flag} --recursive {PKG_PATH}/* tasks.py')
+        check_flag = "--check"
+    if diff:
+        diff_flag = "--diff"
+    c.run(f"{VENV}/bin/black {check_flag} {diff_flag} {PKG_PATH} tasks.py")


This repo doesn't use black.

All the other ones do... It would make sense to have a unified coding standard acorss the board no?

Since plugin authors are volunteering their time and effort to maintain plugins, it's best to honor their wishes in this regard. For background, please read: getpelican/cookiecutter-pelican-plugin#2

I'll go ahead and undo this.

oulenz · 2020-04-20T21:05:32Z

RELEASE.md

+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)


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?

I followed the guide on Pelican's website. See: https://docs.getpelican.com/en/latest/contribute.html#submitting-your-changes

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

So should I leave this one as is for now?

oulenz · 2020-04-23T18:34:42Z

pelican/plugins/more_categories/more_categories.py

+        if category.slug in cats:
+            category.subcategories = cats[category.slug]
+        else:
+            category.subcategories = []


Could you split this up into two attributes, children and descendents (which includes the children)?

Also, please sort them to make things more convenient in the theme templates.

For sure. Do you care which method of sort? Or just the normal sort() method on lists?

oulenz · 2020-04-23T18:37:02Z

pelican/plugins/more_categories/more_categories.py

+    generator._update_context(["categories"])
+
+    # Add subcategories
+    cats = {}


If you initialise this as cats = defaultdict(list), you don't need to test whether it already contains a given category on L86.

oulenz · 2020-04-23T18:40:44Z

pelican/plugins/more_categories/more_categories.py

+        for anc in category.ancestors:
+            if anc != category:


If you iterate from categorate.ancestors[1:], you don't need this check.

oulenz · 2020-04-23T18:44:59Z

pelican/plugins/more_categories/more_categories.py

+                if anc.slug in cats:
+                    cats[anc.slug].append(category)
+                else:
+                    cats[anc.slug] = [category]


You can simply use the category itself as the key, since its hash is the hash of its slug.

thecaligarmo · 2020-04-25T12:44:15Z

@oulenz Ok, I think I fixed everything you wanted and (hopefully) properly reverted the files to their original states in regards to coding style. Let me know if I missed anything.

oulenz · 2020-04-26T10:53:17Z

pelican/plugins/more_categories/more_categories.py

+            if anc == category.parent:
+                children[anc].append(category)


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

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

That's much better. Good idea =D

oulenz · 2020-04-26T10:57:37Z

pelican/plugins/more_categories/more_categories.py

+        if category.slug in descendents:
+            category.descendents = descendents[category]
+        else:
+            category.descendents = []
+        if category.slug in children:
+            category.children = children[category]
+        else:
+            category.children = []


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

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

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

oulenz · 2020-04-26T11:03:06Z

pelican/plugins/more_categories/more_categories.py

@@ -79,6 +78,24 @@ def create_categories(generator):
    )
    generator._update_context(['categories'])

+    # Add subcategories and children


Change to children and descendents

oulenz · 2020-04-26T11:06:39Z

README.md

+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 %}
+


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

oulenz · 2020-04-26T11:20:41Z

tasks.py

+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())


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.

oulenz · 2020-04-26T11:35:13Z

tasks.py

+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')


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]

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.

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?

From what I understand, it should.

oulenz

I've found one remaining issue (which I suggested, sorry!).

Other than that, could you rearrange all commits into three commits?

one with the configuration changes
one with the added functionality and related documentation changes
one with the RELEASE.MD file

If you want, I can also do this for you.

In addition, I've written some tests to check ancestors/descendents functionality, which I'll add.

oulenz · 2020-05-10T13:07:18Z

pelican/plugins/more_categories/more_categories.py

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


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

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)

oulenz · 2020-06-01T10:42:23Z

@thecaligarmo I've rearranged your commits and submitted them via a new pull request. (I've taken care so you're still listed as the author.)

The idea in general is that commits should be relatively self-contained, so that if you are searching through the commit history, it's easier to understand when and why something was changed. It is natural to get feedback on pull request, which leads to new commits. These can be combined by squashing/amending (though not necessarily to just one commit per pull request, there can be more if several things were changed). In addition, it can happen that the changes in your pull request clash with changes added to the master in the meantime, which requires a rebase.

In this case, it took some interactive rebasing to untangle the changes in your commits, so I went ahead and did it myself (also because I haven't used interactive rebasing much before, and I enjoyed the challenge!)

Anyway, many thanks for your contribution!

Aram Dermenjian added 5 commits April 19, 2020 22:11

Version 5.4.1 version of pytest doesn't work with pytest-sugar, and s…

40e0b48

…o force version 5.3.5

Update the tasks file

2dc8c40

Add subcategories to categories and fix formatting

1b5b69e

Added RELEASE.md file

cf8641e

Update READEME

07ddb11

oulenz self-assigned this Apr 20, 2020

oulenz requested changes Apr 20, 2020

View reviewed changes

Aram Dermenjian added 3 commits April 20, 2020 21:12

Put templating in the correct spot

81981a9

Fix version requirement

cac2263

Fix pytest versioning

bf46601

oulenz requested changes Apr 23, 2020

View reviewed changes

Aram Dermenjian added 2 commits April 25, 2020 12:31

Update tasks and fix style

fe04235

Add children and fix code a little

1d83621

oulenz requested changes Apr 26, 2020

View reviewed changes

Aram Dermenjian added 2 commits April 26, 2020 14:28

Update readme and clean up plugin

e57ef22

Use shorter syntax

38c8b80

oulenz requested changes May 10, 2020

View reviewed changes

Fix error

7160f9f

oulenz mentioned this pull request Jun 1, 2020

Add children and descendents #5

Merged

oulenz closed this in #5 Jun 1, 2020

justinmayer mentioned this pull request Aug 20, 2020

Update to namespace plugin format pelican-plugins/jinja-filters#4

Merged

More category fixes #2

More category fixes #2

Conversation

thecaligarmo commented Apr 20, 2020 • edited

oulenz commented Apr 20, 2020

thecaligarmo commented Apr 20, 2020

oulenz commented Apr 20, 2020 • edited

oulenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmayer Apr 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oulenz Apr 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecaligarmo commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oulenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oulenz commented Jun 1, 2020

thecaligarmo commented Apr 20, 2020 •

edited

oulenz commented Apr 20, 2020 •

edited

justinmayer Apr 21, 2020 •

edited

oulenz Apr 23, 2020 •

edited