Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 5 commits
40e0b48
2dc8c40
1b5b69e
cf8641e
07ddb11
81981a9
cac2263
bf46601
fe04235
1d83621
e57ef22
38c8b80
7160f9f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?
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. 🤷♂️
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
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.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.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.
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
anddescendents
(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?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
butconda
.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:
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 cleanertasks.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 usingpoetry 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.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.