cjolowicz / nox-poetry

Use Poetry inside Nox sessions
https://nox-poetry.readthedocs.io/
MIT License
166 stars 18 forks source link

Support installing Poetry dependency groups #1080

Open Niicck opened 1 year ago

Niicck commented 1 year ago

Closes https://github.com/cjolowicz/nox-poetry/issues/663 Closes https://github.com/cjolowicz/nox-poetry/issues/977 Closes https://github.com/cjolowicz/nox-poetry/issues/873

This PR adds a install_groups method to nox_poetry Session proxy. It has the same interface as session.install except that you pass in the dependency groups that you want to install. This feature requires poetry >=1.2.0, but the rest of the code is compatible with <1.2.0, there are no breaking changes.

Example usage: session.install_groups("lint", "test")

Implementation Notes

samypr100 commented 1 year ago

@Niicck I think this also closes #873 due to the changes in src/nox_poetry/poetry.py unless I'm mistaken

samypr100 commented 1 year ago

@Niicck from importlib import metadata is python 3.8+ only, but since importlib-metadata is a dependency (from nox) its possible to use import importlib_metadata as metadata on 3.7 and below as a replacement that way we can keep 3.7 compatibility as stated in the docs for this project

e.g.

try:
    from importlib import metadata
except ImportError:
    import importlib_metadata as metadata

Edit: Only version v3.x+ of importlib-metadata has the py.typed, if you get issues w/ mypy on 3.7 # type: ignore's might be needed if the importlib-metadata package is not high enough (via the dev dependencies which resolves to 1.7.0)

Niicck commented 1 year ago

@samypr100 Big thanks for that check. Tests and mypy are now passing for 3.7

johnthagen commented 1 year ago

Edit: There is now an MR for this:


As a slight tangent, when I was playing around with --with and --only I noticed this Poetry bug:

If anyone was intrepid, fixing that upstream would make using this nox-poetry feature less silently error prone to typos.

johnthagen commented 1 year ago

@cjolowicz Would you be able to review this MR?

Niicck commented 1 year ago

Hello friends. I'm going to be offline for the next couple of months. If there are change requests in future reviews, someone else is welcome to address them and add to this PR.

In the meantime, if people want to install poetry groups using nox, I have a workaround install_poetry_groups function that I use with vanilla nox. It's not unit-tested and it's not as robust as this PR's implementation, but it's functional enough.

Here's the code for that helper function with a couple examples:

def install_poetry_groups(session, *groups: str) -> None:
    """Install dependencies from poetry groups.

    Using this as s workaround until my PR is merged in:
    https://github.com/cjolowicz/nox-poetry/pull/1080
    """
    with tempfile.NamedTemporaryFile() as requirements:
        session.run(
            "poetry",
            "export",
            *[f"--only={group}" for group in groups],
            "--format=requirements.txt",
            "--without-hashes",
            f"--output={requirements.name}",
            external=True,
        )
        session.install("-r", requirements.name)

@nox.session(python=PYTHON_VERSIONS)
@nox.parametrize("django_version", DJANGO_VERSIONS)
def test(session: nox.Session, django_version: str) -> None:
    """Run the pytest suite."""
    args = session.posargs
    install_poetry_groups(session, "main", "test", "dev", "coverage")
    session.install(f"django=={django_version}")
    try:
        session.run("coverage", "run", "--parallel", "-m", "pytest", *args)
    finally:
        if session.interactive:
            session.notify("coverage", posargs=[])

@nox.session(python=PYTHON_VERSIONS[0])
def coverage(session: nox.Session) -> None:
    """Produce the coverage report.

    Combines the results from all test runs from all versions of python. This is because
    some logic branches may only apply to certain versions of python - we want to avoid
    false negative coverage failures when those branches aren't covered by pytest runs
    from other python versions.
    """
    args = session.posargs or ["report"]
    install_poetry_groups(session, "coverage")

    if not session.posargs and any(Path().glob(".coverage.*")):
        session.run("coverage", "combine")

    session.run("coverage", *args)
RomainBrault commented 1 year ago

Any hope to merge this soon @cjolowicz ?

johnthagen commented 1 year ago

@cjolowicz Curious, is nox-poetry something you'd like to continue to maintain, or are you using helper scripts like what you've mentioned before (https://github.com/cjolowicz/nox-poetry/issues/663#issuecomment-1423240512). Would you like to pass nox-poetry off to others in the community who'd like to continue to move it forward?

RomainBrault commented 1 year ago

The minimum should be avoiding warnings (#873) and anticipate future breakage(s).

cjolowicz commented 1 year ago

@cjolowicz Curious, is nox-poetry something you'd like to continue to maintain, or are you using helper scripts like what you've mentioned before (#663 (comment)). Would you like to pass nox-poetry off to others in the community who'd like to continue to move it forward?

@johnthagen I've been swamped with other work, but I expect to circle back to this project in the coming few weeks. I'll do my best to come up with a sustainable model for maintaining this package going forward. That might involve passing it on to others or reducing the bus factor.

johnthagen commented 1 year ago

@Niicck FYI, there are some merge conflicts in this branch now, likely from

johnthagen commented 1 year ago

@Niicck I was curious if you still had any interest in getting this branch back ready for a merge. Would ❤️ this feature.

Niicck commented 1 year ago

@Niicck I was curious if you still had any interest in getting this branch back ready for a merge. Would ❤️ this feature.

If @cjolowicz is interested in approving this PR/feature, then yes I'd be happy to bring the branch back into merge shape. If not, then I won't put any more development time into something that we're not intending to merge.