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

Add Standardize Python project configuration developer guide #1830

Open
wants to merge 11 commits into
base: 6.0
Choose a base branch
from

Conversation

stevepiercy
Copy link
Contributor

@stevepiercy stevepiercy commented Jan 6, 2025

@stevepiercy
Copy link
Contributor Author

First person to review this PR gets a 🍪 !

@mauritsvanrees
@davisagli
@gforcada
@wesleybl
@ericof

@ericof
Copy link
Member

ericof commented Jan 13, 2025

I would like to come back and rediscuss the usage of pre-commit.io in our projects / packages.
It works for Python, it does not for mono repos, and we need to address this before officially recommending the current phone/meta solution for things that are not core packages.
I believe @sneridagh has some opinions about this as well.

@stevepiercy
Copy link
Contributor Author

@ericof plone/meta applies only for Python packages. Because Volto uses an entirely different language and ecosystem, this documentation does not cover that. See:

https://github.com/plone/documentation/pull/1830/files#diff-c5fe55cf1f2b0be4ab5a3a3e79c25c8727dba3e41a0077fdcd5eb7f70c5ba1b4R12-R13

Of course, I would love to have a sibling document, "Standardize Node.js project configuration", or something similarly named.

@ericof
Copy link
Member

ericof commented Jan 13, 2025

I know that much, but for Plone 6 projects (with Volto) or for addons that have both packages (Python and JavaScript), we either have to ignore phone/meta and build something else or we have to maintain a crazy solution with a "fork" of pre-commit.
Currently codebases generated with cookieplone use the second solution, but it is not ideal, and this is why I want to have an open discussion about pre-commit.

@davisagli
Copy link
Member

I would like to come back and rediscuss the usage of pre-commit.io in our projects / packages.
It works for Python, it does not for mono repos, and we need to address this before officially recommending the current phone/meta solution for things that are not core packages.

@ericof That seems off-topic for this PR, which is just trying to document the status quo for how to apply plone/meta to a repository that is not a monorepo and contains only a Python package. Sure, we should work more on it but it's not a blocker here.

@stevepiercy
Copy link
Contributor Author

@ericof could you open an issue in plone/meta, post to the Plone Community Forum, or use Discord to start the discussion you want to have about pre-commit? This PR does not conflict with the issue you describe, and instead documents the current state of standardizing Python packages in Plone.

Also I could perhaps change some text to make it more explicit that the scope of plone/meta is for "Python packages that are not monorepos only".

Please let me know. Thank you!

To lessen the effort of configuring a new project in the Plone GitHub organization, and to keep these projects current with latest configuration practices, the Plone community agreed upon a trusted set of configuration items.
The Plone community manages these configuration items using the [`meta`](https://github.com/plone/meta) project.

You can follow these practices in your own projects, or suggest new or alternative configuration items through the `meta` repository, sharing them with the rest of the Plone community.
Copy link
Member

Choose a reason for hiding this comment

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

I would call it plone/meta or plone.meta through, rather than just meta. There are too many Metas to not disambiguate!

I think we should mention some limitations. It's currently only for use with repositories that have a single Python package at the top level of the repository (so, it can't be used with a monorepo that includes both frontend and backend, like the ones produced by cookieplone). It also doesn't currently work very well for projects that want to support multiple versions of Plone in the same branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1657f29

git clone https://github.com/plone/meta.git
cd meta/config
python3 -m venv venv
. venv/bin/activate
Copy link
Member

Choose a reason for hiding this comment

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

We generally avoid telling people to activate a virtualenv in the docs, because if they don't understand what they're doing it can lead to tricky situations to troubleshoot (i.e. they try to run commands in a new terminal where it isn't activated). So better to leave it inactive and give full paths to the commands in the virtualenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. This was a copy-paste from meta itself. I'll update it here, but I'd like @gforcada to bless this change and consider reusing it upstream.

Done in 302d6e2.

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

This is mostly fine. But it should be updated to be in line with plone/meta#241 and not be merged until that PR is merged. For example, the config directory that is important on the current main branch, will be almost empty or completely gone. See the comment I just added.

@sneridagh
Copy link
Member

@stevepiercy I started a review of the PR, but I stopped because I found out that the wording was wrong all over the place.

To summarize: We need to standardize how do we call things and be consistent all over the place. Just to start with, what is a "Python project" in this context?

@stevepiercy stevepiercy requested a review from davisagli January 14, 2025 20:08
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.

Add meta how-to guide
5 participants