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

Migrate from setup.cfg to pyproject.toml #1243

Merged
merged 1 commit into from Oct 7, 2023

Conversation

azmeuk
Copy link
Contributor

@azmeuk azmeuk commented Oct 7, 2023

I was having a look at #1238 and I realized that setuptools+setup.cfg is not really convenient for build callbacks (such as: catalog compilation).
I was thinking about using hatch instead of setuptools to build packages, like we did recently in wtforms, as it is lightweight and provides build callbacks, and setuptools is deprecated anyways (since it is built on distutils, that have just been deleted in python 3.12).

Anyways, if we use hatch in the future (or flit, or poetry, or whatever), then pyproject.toml is required, and if we keep using setuptools, pyproject.toml won't harm :)

We can have the discussion about the build tool on #1238

console_scripts =
ihatemoney = ihatemoney.manage:cli

paste.app_factory =
Copy link
Contributor Author

@azmeuk azmeuk Oct 7, 2023

Choose a reason for hiding this comment

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

Is paste deployment still used? (I have not migrated this entry point as I am not sure how to migrate this to pyproject.toml)

Copy link
Member

Choose a reason for hiding this comment

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

I've checked and it seems that all the documented deployment options and configuration files are relying on the wsgi module, so we probably don't need to maintain this.

"Documentation" = "https://ihatemoney.readthedocs.io/en/latest/"

[project.scripts]
ihatemoney = "ihatemoney.manage:cli"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the ihatemoney command available.

@@ -32,14 +32,14 @@ def runserver(ctx):
ctx.forward(run)


@click.command(name="generate_password_hash")
@cli.command(name="generate_password_hash")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed to provide ihatemoney generate_password_hash

"Source Code" = "https://github.com/spiral-project/ihatemoney"
"Main instance" = "https://ihatemoney.org/"
"Mobile app" = "https://ihatemoney.org/mobile"
"Documentation" = "https://ihatemoney.readthedocs.io/en/latest/"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[project.urls] allows for more urls to be displayed on the PyPI page.

@almet almet merged commit eb7338c into spiral-project:master Oct 7, 2023
15 checks passed
@azmeuk azmeuk deleted the issue-1238-catalogs branch October 7, 2023 23:56
@zorun
Copy link
Collaborator

zorun commented Oct 8, 2023

Thanks for the work @azmeuk

@almet Could you please let time for other people to review changes before merging them extra-fast? In that case, I would have like to:

  • first add more tests to catch the silent password hash generation crash
  • then merge fix: password generation command line crash #1242 fixing the crash
  • then release a bugfix release 6.1.2
  • then merge more impacting changes that have a risk to introduce regression. Migrating to pyproject.toml does have such a risk (small but anyway)

@almet
Copy link
Member

almet commented Oct 8, 2023

@zorun I'm sorry I didn't let you more time to review this. I understand what you mean and why it could have been simpler to merge it later on.

I believe these scenarii will probably happen more often than before, as more people are working on the repository than usual. We might want to setup some rules for merging, releasing, etc.

I see different ways to solve this, I've open #1245 to discuss that.

TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
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

3 participants