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

Integrating "ambient-package-update" #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GitRon
Copy link

@GitRon GitRon commented Nov 22, 2024

Hi @tim-schilling

here's a very quick-and-dirty example of my https://pypi.org/project/ambient-package-update/ integrated with the playground.

What I've done:

  • I've created a metadata file and configured it
  • I ran the CLI script to update the project

I didn't put too much effort into the details since it's supposed to be a PoC. What do you think about it?

The updater has already some feature flags (migrations, internationalisation) but I'd be open to adding more if this would be a blocker.

Best from Cologne
Ronny

@tim-schilling
Copy link
Member

Thanks @GitRon! What workflows is this looking to streamline for us? There are a lot of changes in this and I'm not sure which were manual and which were automated.

Keep in mind, the primary case for Django Commons is existing libraries with their own unique ways of doing things will be migrated into Django Commons. It's less likely (at this time) that new packages will be implemented directly within Django Commons.

@GitRon
Copy link
Author

GitRon commented Nov 25, 2024

Hi @tim-schilling

What workflows is this looking to streamline for us?

I'm maintaining a bunch of packages. Here's what I do:

  • Make changes in the updater (new Python, new Django, updating linters etc.)
  • Go through all projects and use the updated updater (🤯) to re-render the projects files
  • Check changes per project
  • Commit and release changes

Therefore, the project/package needs to be compatible with the updater. It seems like a lot of changes but in the end if you've got the pattern, it's not so much work. And it will amortize itself in just a couple of update iterations in my experience. Furthermore, we can improve packages. For example, don't ship the testing code in a release etc.

Since we might get any project structure, you are right, it might be a little tricky but I'd be open to changes on the updater itself to accomodate for cases I didn' think about yet.

Hope to clarify this PR.

Best
Ronny

@tim-schilling
Copy link
Member

Hi @GitRon, I haven't forgotten about this. It's a bit much to digest at this moment, but it's on my todo list.

cc @django-commons/admins

@GitRon
Copy link
Author

GitRon commented Dec 2, 2024

Hi @tim-schilling

thx for the heads-up! I think we have a great opportunity here. I think we basically have to find a way to reduce the chores to keep packages up-to-date. I think it's perfectly fine that all existing packages who join django-commons have to go through some kind of migration step. Key is obviously, that the transformation doesn't affect the individual business logic and doesn't take too much time.

Adam Johnson said in a Django chat a while back that he has some kind of tooling for keeping is bazillion packages up-to-date as well. Maybe we could get a second opinion? 🤔

What do you think?

Best
Ronny

@tim-schilling

This comment was marked as outdated.

Comment on lines +1 to +10
from ambient_package_update.metadata.author import PackageAuthor
from ambient_package_update.metadata.constants import (
DEV_DEPENDENCIES,
LICENSE_MIT,
SUPPORTED_DJANGO_VERSIONS,
SUPPORTED_PYTHON_VERSIONS,
)
from ambient_package_update.metadata.maintainer import PackageMaintainer
from ambient_package_update.metadata.package import PackageMetadata
from ambient_package_update.metadata.readme import ReadmeContent
Copy link
Member

Choose a reason for hiding this comment

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

This feels overly verbose and would probably benefit from being something that could be imported as a module.

Copy link
Author

Choose a reason for hiding this comment

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

You mean one single import like

from ambient_package_update.metadata import ReadmeContent, PackageMetadata, PackageMaintainer

this?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more like how django suggests you handle models, from django.db import models

Copy link
Author

Choose a reason for hiding this comment

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

Sounds reasonable. Stick to known pattern. 👌

@@ -0,0 +1,21 @@
MIT License
Copy link
Member

Choose a reason for hiding this comment

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

Can we ignore files entirely? For example, I don't think Django Commons would be specifying the license for every project.

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 a good idea. The updater can already handle two kinds of licences. Why not disable it completely if required. I could integrate this easily.

@tim-schilling
Copy link
Member

tim-schilling commented Dec 5, 2024

I see the merit in a project like this. I think Django Commons can use something like this, though perhaps not like you expect us to. I think the full feature set of ambient-package-update is too broad of scope for how Django Commons will end up operating. Yes, projects will have to change to operate within Django Commons, but it should be a slight change. I think the Metadata class API is a step backwards from the pyproject.toml file. It's an entirely new notation folks will have to understand and it's a bit obtuse right now.

I think a better approach would be to have a metadata file that specifies the configurable bits (supported versions), consume an existing pyproject.toml file, determine what needs to be updated, then update that. A benefit of this approach is that it makes it much easier for an existing project to utilize the tooling. It becomes something to add-on, rather than structure everything around.

I do like the ability to generate a file. We may be able to use this for the GHA release.yml file, the security and code of conduct files and another GHA action check if the security and coc files need an update once per month.

The biggest blockers for me are:

  • I think the pyproject.toml definition is more common and easier for maintainers to add other tooling rather than to have to translate examples to what Metadata wants
  • The Metadata class doesn't feel as organized as a pyproject.toml
  • The documentation for ambient-package-update doesn't feel verbose enough if we need every package maintainer to switch to it

Comment on lines +33 to +35
dependencies=[
f"Django>={SUPPORTED_DJANGO_VERSIONS[0]}",
],
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this shouldn't have to be specified if we're also setting supported_django_versions=SUPPORTED_DJANGO_VERSIONS,

Copy link
Author

Choose a reason for hiding this comment

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

I do understand your point though this is for the pyproject toml file and it defines the "real" dependency when you install the package. This defaults to the lowest version. I added it there so it's explicit and not implicit. Hope my reasoning is understandable?

Copy link
Member

Choose a reason for hiding this comment

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

I understand. I don't think it's clear why you're using the first element of that list. For the reader, they don't know the ordering of that list.

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 true. I'll put this on my list of things I'll work on if we manage to make the "cooperation" work.

@GitRon
Copy link
Author

GitRon commented Dec 6, 2024

Hi @tim-schilling

I think the Metadata class API is a step backwards from the pyproject.toml file. It's an entirely new notation folks will have to understand and it's a bit obtuse right now.

I went with the dataclass approach to be able to add typehints and support people as much as possible. I can understand that you feel this is too custom, though. I wonder if I could add a pyproject.toml-based way of configuring all those things. Shouldn't be too hard. Would this be a promising way to go?

I think a better approach would be to have a metadata file that specifies the configurable bits (supported versions), consume an existing pyproject.toml file, determine what needs to be updated, then update that. A benefit of this approach is that it makes it much easier for an existing project to utilize the tooling. It becomes something to add-on, rather than structure everything around.

This seems like the answer to my question one paragraph before 😅

I do like the ability to generate a file. We may be able to use this for the GHA release.yml file, the security and code of conduct files and another GHA action check if the security and coc files need an update once per month.

Cool! At first I went with a static approach meaning I define all the files and if you don't like it you can change it in your project/package after using the renderer. But a friend added the possibility for custom templates which you can overwrite in your project. I'm really not decided if I like this or not. If we find a way to marry the updater to django-commons, I'd be happy about some thoughts on this topic.

I think the pyproject.toml definition is more common and easier for maintainers to add other tooling rather than to have to translate examples to what Metadata wants

I could add this 👌

The Metadata class doesn't feel as organized as a pyproject.toml

Agree to disagree. I find a pyproject.toml quite confusing and prefer Python dataclasses. But as I said before, I think we can find a solution here.

The documentation for ambient-package-update doesn't feel verbose enough if we need every package maintainer to switch to it

That's because we just have a handfull of users. It's kinda hard to write down what others might not know or understand without being able to ask someone 😅 I'd be more than willing to invest in the docs if we decide to use this in django-commons.

What's your feeling about this? Do you think we'll find a way to make this work?

Best from Cologne
Ronny

@Stormheg
Copy link
Member

Hello Ronny,

Thank you for the proposal. As Tim mentioned, we are interested in automating some of the 'boring' parts of package maintenance.

At my day job we use a tool on top of cookiecutter called cruft. I believe it works by trying to apply git diffs of commits made to your project template repository to a particular project repository. This works best if projects don't deviate too much from said cookiecutter template, otherwise you'll end up with a lot of git conflicts that one has to resolve. That leaves you only with a 'consistency' benefit, as resolving those conflicts can be quite a chore. For that reason, cruft might not be particularly well suited for Django Commons as most projects are quite unique. For projects that are bootstrapped from a project template and stick to it closely, it would work much better.

I think roughly the same applies to ambient-package-update: you need to make significant changes to your project to adopt it and 'buy-in'. That's not necessarily a bad thing, but it is likely not something we will ever require of projects.

Overall, my impression so far is that ambient-package-update is quite opinionated. Like Tim, I think the Metadata API is a step backwards. To me it feels like an unnecessary layer on top of pyproject.toml. I don't think pyproject.toml is confusing to use on its own. It's also something you would typically have to set up only once and after you don't often make changes to it.

I think a better approach would be to have a metadata file that specifies the configurable bits (supported versions), consume an existing pyproject.toml file, determine what needs to be updated, then update that. A benefit of this approach is that it makes it much easier for an existing project to utilize the tooling. It becomes something to add-on, rather than structure everything around.

I approve of this suggestion by @tim-schilling - a tool to quickly parse and change some specific parts of a pyproject.toml or tox.ini file sounds more like a worthwhile approach to support existing projects.


I realize this is not something you would be keen to hear as author of ambient-package-update, but this is how I feel about it.

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.

3 participants