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 support for pyproject.toml files when adding --editable projects … #7670

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ughstudios
Copy link

There is currently a problem where setup_reader.py can only read setup.cfg or setup.py files for adding local packages. This adds support for flitcore and other backends that are defined in pyproject.toml files

Resolves: #7669

…that are on local disk.

There is currently a problem where setup_reader.py can only read setup.cfg or setup.py files for adding local packages. This adds support for flitcore and other backends that are defined in pyproject.toml files
@dimbleby
Copy link
Contributor

dimbleby commented Mar 17, 2023

immediate thoughts

  • general idea seems good, pep-621 compliant pyproject.toml are beginning to appear in the wild and it's good to get a first step in supporting them
  • this code certainly doesn't want to be in a class called SetupReader, it's not dealing with setup files, suggest instead introducing something new and refactoring PackageInfo.from_directory()
  • I don't think this should be tied to flit: pep 621 is a standard and other backends support it
  • I'd anti-recommend using tomlkit to parse pyproject.toml here. poetry already has tomllib in its dependencies - that'll be faster and also sidestep the type-checking issues currently visible in the pipeline
  • needs testcases

@ughstudios
Copy link
Author

I'd anti-recommend using tomlkit to parse pyproject.toml here. poetry already has tomllib in its dependencies - that'll be faster and also sidestep the type-checking issues currently visible in the pipeline

The codebase I work on is only 3.10, do you mind if I add a fallback to use tomlkit for versions <3.11? tomllib is only available in 3.11.

@dimbleby
Copy link
Contributor

The codebase I work on is only 3.10, do you mind if I add a fallback to use tomlkit for versions <3.11? tomllib is only available in 3.11.

from poetry.utils._compat import tomllib

@ughstudios
Copy link
Author

ughstudios commented Mar 17, 2023

Is there a way to run these tests at my desk so I don't keep spamming you? NVM I guess I can just run each of them manually.

@ughstudios
Copy link
Author

I'm running through all of the tests in the test suite. Are any of the tests known to be broken? It seems like my change has broken like 10+ tests :(

@dimbleby
Copy link
Contributor

If you've figured out how to run the tests then you can very easily verify for yourself that they pass without your changes.

On the plus side this probably points you in the right direction for figuring out where to add new tests.

Copy link

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

Looks good. I think the test failures are just due to the test suite relying too much on mocks, making it fragile when we touch the internals. I am also getting them in #7703, but haven't gotten around to look at them.

@@ -498,6 +499,7 @@ def from_directory(cls, path: Path, disable_build: bool = False) -> PackageInfo:
else:
info = get_pep517_metadata(path)
except PackageInfoError:
info = cls.from_pyproject_toml(path)
Copy link

Choose a reason for hiding this comment

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

IMO, instead of putting this here, we should probably create a from_project_static_files which tries from_pyproject_toml and then from_setup_files, and then replace all existing from_setup_files usages with from_project_static_files.

Copy link
Author

Choose a reason for hiding this comment

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

I'll have to do this on Monday when I'm back at my desk. Are you one of the maintainers of poetry? It says "FFY00 approved these changes 3 days ago."

@dimbleby dimbleby mentioned 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.

Support reading metadata from PEP 621 compliant pyproject.toml
3 participants