-
Notifications
You must be signed in to change notification settings - Fork 64
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 Poetry #402
base: main
Are you sure you want to change the base?
Conversation
Thanks a ton for the PR @orsinium! I'll do a review in a moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really great start! Thanks a ton, again, for tackling this feature.
I've highlighted a couple of small nits, plus a big design question that needs to be resolved.
from pip_audit._dependency_source import PoetrySource | ||
from pip_audit._service import ResolvedDependency, SkippedDependency | ||
|
||
TEST_DATA_PATH = Path(__file__).parent / "data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another tiny nitpick: could you turn this into an asset
or test_data
fixture and put it in conftest.py
?
Here's the pattern we use on a handful of other projects:
# conftest.py
_ASSETS = (Path(__file__).parent / "assets").resolve()
assert _ASSETS.is_dir()
@pytest.fixture
def asset():
def _asset(name: str) -> Path:
return _ASSETS / name
return _asset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could. But why? This one is short, type-safe, and not coupled on conftest.py. I love good autocomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main reason is potential reuse: we have a lot of other tests (particularly the requirements file ones) that would benefit from being broken out into stand-alone assets, so having a centralized "asset" fixture would keep things unified.
Separately (and I admit I have no idea if this is "good" practice or not): my understanding is that pytest
conceptually encourages each test module to contain nothing except test code, with conftest.py
being the idiomatic place to put any scaffolding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a lot of other tests (particularly the requirements file ones) that would benefit from being broken out into stand-alone assets
YAGNI ;)
Separately (and I admit I have no idea if this is "good" practice or not): my understanding is that pytest conceptually encourages each test module to contain nothing except test code
Never heard of that. Test files can contain fixtures, and from my experience, they should when the fixture is used only in that file. Having all fixtures in conftest.py
brings unnecessary coupling, makes it harder to track which fixtures aren't needed anymore, and that file being a typical example of "God class" (in this case, God module) can grow to some crazy numbers (last month, I decoupled 10k lines conftest.py). And even if we assume "keep all fixtures in conftest.py", it's not a fixture. It's literally a constant.
Having a constant has direct benefits: it's shorter, type-safe, closer to the code (easier to unravel the test code), and makes the test file self-contained. If none of that persuades you, please, just say it, and I'll update the PR. I want the code I contribute to be nice and readable, but the final word is always on the direct maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM structurally. This PR changes a user-facing component, so we'll need a CHANGELOG entry -- could you add that @orsinium? 🙂
I think merging this is probably blocked on making a determination on whether this tool should support Poetry in any capacity in the long-term. The original/current intention that this will become part of That discussion should probably happen in #84 before we do any more work on this or merge it. |
If the project path is specified in CLI, read dependencies from
poetry.lock
file (if available).I'm quite happy with
collect
. Since this is a lock file, we can safely skip dependency resolution, as we do forpip
source. And the format of the file is quite simple, so we don't depend on poetry for reading it. Cool!Autofix is not supported yet because poetry needs hashes to be updated, see review comments.
I checked the change manually on a few quite big poetry projects, works like a charm.
Close #84