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

Update to namespace plugin format #4

Merged
merged 13 commits into from Aug 20, 2020
Merged

Update to namespace plugin format #4

merged 13 commits into from Aug 20, 2020

Conversation

justinmayer
Copy link
Collaborator

Following up on #3, I used the Cookiecutter template to scaffold a new plugin, using the generated output as the basis for making the changes found in this pull request.

@justinmayer
Copy link
Collaborator Author

@MinchinWeb: I made some changes aimed at consistency with the other plugins under this new organization, including CI-powered automatic publishing to PyPI. What do you think?

Copy link
Member

@MinchinWeb MinchinWeb left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @justinmayer !

One of the biggest changes is the change in the build/release process. These are my concerns, help put me to ease on them please:

  1. The old setup.py pulled much of the metadata (but particularly the version) from the same place the internal code did. Now this is kept in multiple places (in the plugin and the poetry configuration); how to we ensure that they are kept in sync?
  2. Does poetry need to explicitly list namespace packages? (This was needed in the old setup).
  3. How do we keep tasks.py up-to-date and in-sync across all the expected plugins? I was doing this using an external project (Minchin dot Releaser), but I'm not sure we could do the same here (though that would be my first inclination) as the proposed tasks.py sets up the development environment, so I'm not sure if it can practically rely on an external package in the same way...
  4. Is there an automated way to keep the version of the linting tools listed in the pre-commit config in sync with the versions listed in the poetry config?
  5. Minchin dot Releaser allowed me to do the full release process from a single command. Are we moving that to a GitHub action?
  6. Minchin dot Releaser had a couple of checks that I don't see here... are these easy to replicate?
    • check that the Readme will render on PyPI
    • check that the built distribution can be installed and loaded correctly (checked by asking for the package version; this was designed to catch very basic packaging mistakes)
    • create a git tag for the release

Probably beyond the scope of this pull request, it would be cool if this plugin's documentation could be folded into the main Pelican documentation somehow....

CHANGELOG.rst Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@justinmayer
Copy link
Collaborator Author

Many thanks for the thoughtful and detailed feedback. I know there are a lot of changes in here, many of which I did not sufficiently explain in the pull request description. Mea culpa. Following below, I will try my best to answer your questions…

  1. The idea is that the version number stored in pyproject.toml is the “source of truth”. In most cases, the version number doesn’t need to be stored anywhere else, and thus there is no need to keep the version number in sync. That said, because some legacy projects have traditionally stored version number strings in other places that are subsequently imported and used elsewhere in the project, I added a feature to AutoPub that will automatically update those extra version strings when the version number is incremented during CI-powered automatic releases. In a structure where nearly all metadata is stored in pyproject.toml, I’m not sure whether/how the metadata in jinja_filters/__init__.py continues to be useful, and therefore it’s not clear to me that we even need to try to keep that information in sync. It seems to me that information could be removed from jinja_filters/__init__.py, but feel free to let me know if there’s a good reason to keep it there.

  2. I’m not sure if I understand this question, but no, I don’t believe so. The following line should be the relevant bit that handles the namespace plugin: https://github.com/pelican-plugins/jinja-filters/pull/4/files#diff-522adf759addbd3b193c74ca85243f7dR12

  3. As you suggested, I’m not sure tasks.py can safely be abstracted and subsequently used for all plugins, since some plugins may have specific task requirements that differ from others. But this question is still valid for the many other files generated by the Cookiecutter plugin, which nearly all plugins will use unchanged. For now, I’ve been manually committing and pushing updates on an ad-hoc basis — I haven’t determined whether there is a more automated way to keep them all up-to-date. I toyed with Nitpick briefly but haven’t had sufficient time to evaluate its feasibility for this purpose.

  4. I’m not aware of an automated way to keep the version of the linting tools listed in the Pre-commit config in sync with the versions listed pyproject.toml. That said, these versions don’t usually need to be updated frequently, and updating them in both places — while sometimes easy to forget — isn’t particularly difficult.

  5. The other plugins under the new organization are automatically published to PyPI via GitHub Actions + AutoPub, so I did indeed intend to do the same with Jinja Filters. You read more about the rationale here: https://justinmayer.com/projects/autopub/ That said, if necessary, it’s easy to also manually publish to PyPI via a single command: poetry publish

  6. AutoPub automatically creates the Git tag for the release. The two checks you mentioned seem like good ideas that perhaps could be added to AutoPub.

[…] it would be cool if this plugin's documentation could be folded into the main Pelican documentation somehow

I overlooked updating the installation instructions in the README. I just pushed a commit that updates it and fixes a few things, too. I don’t know if it makes sense for plugin documentation to be separated from its repo and instead be stored in Pelican core’s repository. Could you perhaps elaborate regarding why that would be beneficial?

@MinchinWeb
Copy link
Member

Thanks for working through this.

  1. Re metadata in __init__.py: I think __version__ is the only one regularly used. I find it helpful sometimes when debugging to figure out if I'm accessing the version of the code that I think I am. As for the rest, it's used much less (if at all), so probably could be dropped, but also is much less likely to change, so maybe it's not a concern to leave it. Your call, but I'd be fine with removing all but __version__.

  2. Sounds good!

  3. What if everything but project set up is abstracted? Have a tasks.py file that looks something like this:

    from invoke import task
    
    # these are specific to this package
    PKG_NAME = "jinja_filters"
    PKG_PATH = Path(f"pelican/plugins/{PKG_NAME}")
    
    try:
        # final name to be determined...
        from pelican.tasks import *
    except ImportError:
        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())
    
        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")
            
        TOOLS = ["poetry", "pre-commit"]
    
        @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"{VENV}/bin/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"{VENV}/bin/pip install -U pip")
            tools(c)
            c.run(f"{POETRY} install")
            precommit(c)
    
    # other, project specific tasks

    So the basic idea is that the tasks.py file would contain enough to set up the virtual environment, and let the project be installed, but anything more would be available after install and upgrade-able through a common "tasks" package (here assumed to be named pelican.tasks. As the project would maintain a tasks files, project specific tasks can still be added. I'm sure with a little bit of thought about workflow, this file could be trimmed even further.

    Anyway, I think have a common tasks file would be a great gain in to having the plugins under a common organization.

  4. Sounds good for now.

  5. Sounds good. I like the idea of frequent and simple releases. I guess autopub integration will be a follow on pull request? As well, we may want to add a GitHub Action that checks for the changelog stub autopub looks for.

  6. Sounds good.

  7. Re "folding" the documentation: No, I think this plugin should have it's documentation hosted here, so hopefully it stays up to date. More the thought was that when the main Pelican core documentation is generated, it would pull this in plugin's documentation (along with any other "official" plugins) and then the documentation for the Plugins as a whole would show up as a section in the core Pelican documentation. The hope would be to increase discoverablity and highlight the strength of that the plugins bring to Pelican.

    As well, plugins don't need a lot in terms of documentation, but a little is still needed. So it's kind of overkill to set up a "proper" documentation site, and so by making it part of the core documentation, that becomes one less thing to worry about when dealing with each plugin.

@justinmayer
Copy link
Collaborator Author

@MinchinWeb: Thank you for the detailed notes. I’ll comment on what I think are the only outstanding items left…

  1. I added a commit that should automatically update the __version__ variable in __init__.py: 9b06e20

  2. I think that sounds like it could be worth trying. Do you mind if we add the current tasks.py for now and then replace it once we have the abstracted module ready to use? I’d prefer not to hold up the publication of the namespace-ready version of Jinja Filters to PyPI. (See below for more info.)

  3. I’m not sure that any plugin needs any more documentation than what can be briefly listed in its README, with any information not specific to the plugin contained in Pelican core’s documentation. That said, I’m certainly open to exploring this idea further.

With all that said, is there anything left to do before this is merged? Once this has been merged, we should be ready to publish the namespace-ready version to PyPI. Hopefully we can do that within the next 24 hours, as I'm hoping to release the next version of Pelican — the first release that supports namespace plugins — tomorrow, Thursday, Aug. 20th. Looking forward to having the namespace-ready version of the Jinja Filters plugin published and ready by that time! 💫

This was referenced Aug 20, 2020
@MinchinWeb MinchinWeb merged commit 0e973f7 into master Aug 20, 2020
@MinchinWeb MinchinWeb deleted the modernize branch August 20, 2020 03:07
@MinchinWeb
Copy link
Member

MinchinWeb commented Aug 20, 2020

@justinmayer It's done! I created a couple of issues for the few outstanding things that we talked about, but weren't blocking. I think the only piece left is for you to turn on autopub, and once version 2.0.0 is out, I'll release one more entry on the 1.x line to point people to the new location.

Good luck on the Pelican 5.0 release!

@justinmayer
Copy link
Collaborator Author

justinmayer commented Aug 20, 2020

Great! Glad to see this merged. 🎉

By nature of the relevant configuration, AutoPub should already be set up. I'll release version 2.0.0 as soon I get the Pelican release out the door.

(Speaking of which, the plan is for the next Pelican release to be 4.5.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants