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: Implement --annotation-style parameter for uv pip compile #1679

Merged

Conversation

DrJackilD
Copy link
Contributor

Summary

Hello there! The motivation for this feature is described here #1678

Test Plan

I've added unit tests and also tested this manually on my work project by comparing it to the original pip-compile output - it looks much like the pip-compile generated lock file.

@DrJackilD DrJackilD changed the title Implement --annotation-style parameter for uv pip compile feat: Implement --annotation-style parameter for uv pip compile Feb 19, 2024
@zanieb
Copy link
Member

zanieb commented Feb 19, 2024

Thanks for contributing! Welcome to the project :)

I'm not entirely sure we want to accept more configuration here, I'll need to check with @charliermarsh.

It looks like the Windows snapshots are failing. We automatically filter out some Windows-only dependencies, but it's failing since they're not separated by a newline anymore. You'll need to adjust the regex at

let windows_only_deps = [
("( [+-] )?colorama==\\d+(\\.[\\d+])+\n( # via .*\n)?"),
("( [+-] )?tzdata==\\d+(\\.[\\d+])+\n( # via .*\n)?"),
];

@zanieb zanieb added the configuration Settings and such label Feb 19, 2024
@DrJackilD
Copy link
Contributor Author

@zanieb I hope you'll accept this one :) Regarding the tests - fixed!

}
}
}
// Assemble the line with the annotations and remove trailing whitespaces.
line = format!("{line:24}{sep}{annotation}")
Copy link
Member

Choose a reason for hiding this comment

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

Can you walk me through line:24 here?

Copy link
Contributor Author

@DrJackilD DrJackilD Feb 20, 2024

Choose a reason for hiding this comment

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

Sure! I'm trying to keep the same logic, as pip-tools, to minimize the diff in the output file. The align 24 is something they use in the pip-tools as a reasonable number to align the annotation comments: https://github.com/jazzband/pip-tools/blob/main/piptools/writer.py#L318 and I agree with them. Most of the time annotation will be aligned except for some extra-long dependencies.

monotonic==1.6            # via analytics-python
oauthlib==2.1.0           # via pyproject.toml
opentelemetry-api==1.22.0  # via ddtrace
packaging==21.3           # via gunicorn, pyproject.toml
prompt-toolkit==3.0.43    # via click-repl
protobuf==4.25.3          # via ddsketch, ddtrace

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay, thank you, perfect!

I'm wondering if we should just put more of this method within a match on the annotation style. I might find the logic easier to follow, but I'd need to see it to know for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to extract annotation_line and annotation_split onto the match statement? I could do this, they're not very big, should be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right, it's easier to read without those function calls :)

@charliermarsh
Copy link
Member

I'm happy to include this.

@DrJackilD DrJackilD force-pushed the feature/implement-annotation-style branch from 00f6ddb to 4d71213 Compare February 20, 2024 23:12
@DrJackilD
Copy link
Contributor Author

DrJackilD commented Feb 20, 2024

I have no idea, why it's failing 🤔

UPD: Fixed 😊

@DrJackilD DrJackilD force-pushed the feature/implement-annotation-style branch from 4d71213 to 3a119e3 Compare February 20, 2024 23:34
@charliermarsh
Copy link
Member

Will review and merge tonight, hopefully.

@charliermarsh charliermarsh force-pushed the feature/implement-annotation-style branch 5 times, most recently from 3ec4eee to 8c82063 Compare February 21, 2024 01:54
@charliermarsh charliermarsh enabled auto-merge (squash) February 21, 2024 01:54
@charliermarsh charliermarsh force-pushed the feature/implement-annotation-style branch 2 times, most recently from fe16f34 to 5d579e1 Compare February 21, 2024 01:56
}
}

if let Some((separator, comment)) = annotation {
Copy link
Member

Choose a reason for hiding this comment

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

@DrJackilD -- I just tweaked this to couple the presence of the comment and separator.

@charliermarsh charliermarsh force-pushed the feature/implement-annotation-style branch from 5d579e1 to d22aa3e Compare February 21, 2024 02:00
@charliermarsh charliermarsh merged commit 31752bf into astral-sh:main Feb 21, 2024
7 checks passed
@charliermarsh
Copy link
Member

Thank you! Great contribution.

@DrJackilD
Copy link
Contributor Author

DrJackilD commented Feb 21, 2024

Thank you! Great contribution.

Thank you for accepting it so quick!

@DrJackilD DrJackilD deleted the feature/implement-annotation-style branch February 21, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Settings and such
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants