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

Deduplicate editables during install commands #2820

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

charliermarsh
Copy link
Member

Summary

Closes #2819.

@charliermarsh charliermarsh added the bug Something isn't working label Apr 4, 2024
@charliermarsh charliermarsh marked this pull request as ready for review April 4, 2024 16:46
Comment on lines 3105 to 3107
-e file://../../scripts/packages/black_editable
-e file://../../scripts/packages/black_editable
-e file://../../scripts/packages/black_editable[dev]
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 add test coverage for an absolute path that resolves to the same as a relative path?

Should we add handling for symbolic links?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer not to merge symlinks here, but I can add an absolute path test, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, we could merge symlinks, it's not difficult, I'm just not sure if it's correct. I guess in theory they must resolve to the same package, so maybe it's totally fine...

@charliermarsh charliermarsh merged commit ab8368a into main Apr 4, 2024
35 checks passed
@charliermarsh charliermarsh deleted the charlie/dupe branch April 4, 2024 17:19
@jaap3
Copy link

jaap3 commented Apr 4, 2024

Absolutely sublime🥇. Thanks for thinking about extra's as well. That's actually something we use and I forgot to mention it in the initial report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dedupe editable installs before building
3 participants