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: normalize package names where applicable #285

Merged
merged 12 commits into from
Aug 28, 2023

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented Aug 22, 2023

This PR fixes an issue where package names of conda are always lowercase but they don't have to be lowercase in specs. This PR introduces a new type PackageName that holds both the original source package name as well as the normalized (lowercased) package name. The type is used throughout the rest of the code base which makes this a slightly breaking change.

This should fix the issue reported here.

@wolfv
Copy link
Contributor

wolfv commented Aug 23, 2023

Not really sure if we need to keep the original form or not. And my second question would be that if we do this for all MatchSpecs, are we going to loose a lot of performance (in repodata we can assume that they are already lowercase, I guess).

@baszalmstra
Copy link
Collaborator Author

Yeah I was also wondering if it is useful to keep the source representation. I was also thinking that it might be good to use PackageName in PackageRecord as well. So that we use the same type where we semantically want to refer to a package name. @wolfv WDYT? We can also have a conversion method where we don't check if the name contains uppercase characters and we just assume its valid.

The overhead of PackageName is really small but it does increase memory size slightly a little bit.

@baszalmstra baszalmstra marked this pull request as draft August 23, 2023 09:21
@twrightsman
Copy link

@baszalmstra Thanks for working on this! I am also unsure if it is useful within rattler to keep the source representation. As a user, the behavior I'd like to see is pixi not normalizing the name in pixi.toml whether it was added manually or through things like pixi add. So if rattler doesn't track the source representation, then I suppose pixi would have to know how to map unnormalized names provided by the user to anything normalized returned by rattler? Just my two cents 😄

@baszalmstra
Copy link
Collaborator Author

baszalmstra commented Aug 24, 2023

I changed this PR quite a bit. PackageName is now used everywhere we are semantically talking about a package name. I also added validation that a PackageName only contains valid characters. My gut says the overhead of the implementation is very low but I didn't test it.

@baszalmstra baszalmstra marked this pull request as ready for review August 24, 2023 14:16
@tdejager tdejager merged commit 58baa21 into conda:main Aug 28, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants