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 pyproject.toml as default input file format #1780

Merged
merged 18 commits into from
Dec 24, 2022
Merged

Add pyproject.toml as default input file format #1780

merged 18 commits into from
Dec 24, 2022

Conversation

berislavlopac
Copy link
Contributor

@berislavlopac berislavlopac commented Dec 15, 2022

Added pyproject.toml as a default input file name that is searched for when pip-compile is invoked without arguments; see #1779 for details.

The default file names are searched in the following order, with the first encountered being used to compile:

  1. requirements.in
  2. setup.py
  3. setup.cfg
  4. pyproject.toml

Closes #1779.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev added the bug Something is not working label Dec 17, 2022
@ssbarnea ssbarnea self-requested a review December 17, 2022 16:06
@ssbarnea
Copy link
Member

@berislavlopac I a very happy to see your proposed that. I was planning to do it myself for quite a few weeks but failed to find the time. Now you can be sure I will do my best to get this merged.

@ssbarnea
Copy link
Member

@berislavlopac Apparently there is a reduction in codecoverage reported by codecov, one that seems to be genuine.

@berislavlopac
Copy link
Contributor Author

@ssbarnea I'm confused - it looks like codecov is complaining that the lines in the test file were not covered by tests? 🤔

@webknjaz
Copy link
Member

@berislavlopac yes, from https://app.codecov.io/gh/jazzband/pip-tools/pull/1780 it's evident that the else-block code branch doesn't get executed, meaning that fname in default_file_names is always true there. At least in the CI.

@webknjaz
Copy link
Member

Looking at the diff, it's probably related to the fact that you test against CWD which is a bad idea. Look at how other tests create temporary directory structure for all the cases they cover.

default_file_names = ("requirements.in", "setup.py", "pyproject.toml", "setup.cfg")
make_module(fname=fname, content=content, base_dir=os.getcwd())
out = runner.invoke(cli)
if fname in default_file_names:
Copy link
Member

Choose a reason for hiding this comment

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

Usually, the tests should be simple and test one thing only. Once logic is added, they tend to become hard to read/understand as well as easy to be misleading/buggy.

@berislavlopac
Copy link
Contributor Author

@webknjaz I agree with your points, and will look into the alternative to cwd. However, the issue appeared because setup.cfg was added to the list of allowed files.

I'm splitting the test into two separate cases, one for when there is an allowed file and another without it.

That said, I would still argue that we don't need to include setup.cfg in the list of automatically accepted files. This is because -- if I'm not mistaken -- on its own it doesn't do anything; it either requires setup.py or pyproject.toml to be present for setuptools to work properly.

@webknjaz
Copy link
Member

on its own it doesn't do anything; it either requires setup.py or pyproject.toml to be present for setuptools to work properly.

It does require those, but it can still contain the deps list on its own. I don't remember exactly, but I think that pip-tools might be loading it directly.

@berislavlopac
Copy link
Contributor Author

Okay, after further thought I am okay with keeping in the explicitly defined default list, especially if it's listed after the other two. 👍

@berislavlopac
Copy link
Contributor Author

you test against CWD which is a bad idea

Since this test is for the functionality that depends directly on some files being present (or not) in the CWD, I'm not sure what would be the right way to test it, apart from creating those files in the CWD (which the current tests do)? 🤔

@webknjaz
Copy link
Member

@berislavlopac
Copy link
Contributor Author

berislavlopac commented Dec 21, 2022

@webknjaz Done, at the cost of a couple more fixtures added to the test. 😄

tests/conftest.py Outdated Show resolved Hide resolved
ssbarnea and others added 2 commits December 24, 2022 17:27
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A few comments.

tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
@atugushev atugushev added this to the 6.12.2 milestone Dec 24, 2022
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you very much for your patience!

@atugushev atugushev enabled auto-merge (squash) December 24, 2022 23:31
@atugushev atugushev changed the title Add pyproject.toml as default file format (#1779) Add pyproject.toml as default input file format Dec 24, 2022
@atugushev atugushev merged commit cee88ed into jazzband:main Dec 24, 2022
@atugushev atugushev linked an issue Dec 25, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pyproject.toml as a default file name option
4 participants