-
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
More category fixes #2
Conversation
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. |
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. |
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.) |
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'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).
@@ -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 |
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.
Why 4.2.0? (3.8 became 4.0, but that should be enough.)
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 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?
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.
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.
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.
@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?
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.
@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. 🤷♂️
@@ -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 |
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.
Please don't change single quotes to double quotes.
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.
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.
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.
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. 😊
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 cool. I'll go ahead and undo those then. Sorry about that @oulenz
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()) |
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 prefer activating a suitable virtual environment manually. In particular because I (and maybe others) don't use virtualenv
but conda
.
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 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 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. 😉
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. Sorry .-. I'll go ahead and undo this as well.
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 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?
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 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? 🤔
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, 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 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.
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 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.
tasks.py
Outdated
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") |
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.
This repo doesn't use black.
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.
All the other ones do... It would make sense to have a unified coding standard acorss the board no?
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.
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
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'll go ahead and undo this.
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) |
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?
if category.slug in cats: | ||
category.subcategories = cats[category.slug] | ||
else: | ||
category.subcategories = [] |
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.
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.
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.
For sure. Do you care which method of sort? Or just the normal sort()
method on lists?
generator._update_context(["categories"]) | ||
|
||
# Add subcategories | ||
cats = {} |
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.
If you initialise this as cats = defaultdict(list)
, you don't need to test whether it already contains a given category on L86.
for anc in category.ancestors: | ||
if anc != category: |
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.
If you iterate from categorate.ancestors[1:]
, you don't need this check.
if anc.slug in cats: | ||
cats[anc.slug].append(category) | ||
else: | ||
cats[anc.slug] = [category] |
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.
You can simply use the category itself as the key, since its hash is the hash of its slug.
@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. |
if anc == category.parent: | ||
children[anc].append(category) |
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.
This is slightly inelegant, could you place it outside the inner loop, and change it to:
if category.parent:
children[category.parent].append(category)
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 much better. Good idea =D
if category.slug in descendents: | ||
category.descendents = descendents[category] | ||
else: | ||
category.descendents = [] | ||
if category.slug in children: | ||
category.children = children[category] | ||
else: | ||
category.children = [] |
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.
Since we're using defaultdicts, we can simply do:
category.descendents = sorted(descendents[category])
category.children = sorted(children[category])
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'm not used to defaultdicts .-. I really need to get used to them though cause that's so much nicer. =D
@@ -79,6 +78,24 @@ def create_categories(generator): | |||
) | |||
generator._update_context(['categories']) | |||
|
|||
# Add subcategories and children |
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.
Change to children and descendents
README.md
Outdated
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 %} | ||
|
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.
"category.children
and category.descendents
", and change the example to use category.children
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()) |
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 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.
tasks.py
Outdated
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') |
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 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]
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.
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.
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 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?
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.
From what I understand, it should.
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'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.
descendents = defaultdict(list) | ||
children = defaultdict(list) | ||
for category, articles in generator.categories: | ||
for anc in category.ancestors[1:]: |
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, this should be category.ancestors[:-1]
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.
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)
@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! |
This fix does two things:
category.subcategores
which is alist
of categories which containcategory
as a parent.