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

feat: Make project table optional for pyproject.toml manifests #1732

Merged

Conversation

olivier-lacroix
Copy link
Contributor

@olivier-lacroix olivier-lacroix commented Aug 4, 2024

This Fixes #1687 and #1207 by keeping the [project] table optional in pyproject.toml.

The [project] table had been made mandatory to make sure a name was defined. Now, the project name is read from, in order of priority

  • the [tool.pixi.project] table
  • the [project] table
  • the [tool.poetry] table

The same is done for project description, version and authors.

During init, if the name cannot be read from the [project] table or the [tool.poetry] table, a default name (inferred from the directory name) is added to the [tool.pixi.project]

This PR also refactors handling of pyproject.toml: both pixi and non-pixi pyproject.toml are now read into the same PyProjectManifest struct, with existence checking of the [tool.pixi.project] table managed outside of serde when necessary.

@olivier-lacroix olivier-lacroix changed the title Make project table optional for pyproject.toml manifests feat: Make project table optional for pyproject.toml manifests Aug 4, 2024
@olivier-lacroix olivier-lacroix force-pushed the feat/optional-project-table branch 4 times, most recently from 9434b92 to 4962401 Compare August 4, 2024 02:21
@olivier-lacroix olivier-lacroix marked this pull request as ready for review August 4, 2024 02:29
@olivier-lacroix olivier-lacroix force-pushed the feat/optional-project-table branch 5 times, most recently from 19dd54d to 704d02f Compare August 4, 2024 05:22
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Thanks !! Looks good to me! And works like a charm.

@ruben-arts
Copy link
Contributor

Asked @tdejager for another quick review.

Copy link
Contributor

@tdejager tdejager left a comment

Choose a reason for hiding this comment

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

LGTM! The only thing I would change, is maybe if the name does not exist add it to the pyproject project table instead of the tool.pixi this makes it more standard-compliant. The only downside I can see is if you want to not use that name for a wheel build, but that would be pretty strange. WDYT @olivier-lacroix ?

@tdejager
Copy link
Contributor

tdejager commented Aug 5, 2024

With some closer look, it seems to be a bit difficult to do this, I would like it but lets postpone, I'll approve and merge as is for now.

@ruben-arts ruben-arts merged commit a5bf27b into prefix-dev:main Aug 5, 2024
24 checks passed
@olivier-lacroix
Copy link
Contributor Author

Thx @tdejager ! Maybe useful to have @pavelzw thoughts on this as well: I think I remember him not liking pixi imposing the creation of that table.

I do not see any difficulty in implementing it though: if project.name does not exist, this means there is no project table at all (as the field is mandatory), so we can 'just' insert it before the [tool.pixi] section. Or am I missing something?

@olivier-lacroix olivier-lacroix deleted the feat/optional-project-table branch August 5, 2024 11: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
3 participants