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

Warn when override of a default package uses the incorrect case #708

Open
wants to merge 6 commits into
base: branch-24.12
Choose a base branch
from

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Oct 24, 2024

Description

It is a common mistake to presume that all entries in a package override json should use lower case names, since the vast majority of default packages are all lower case. This causes silent 'failures' when the override doesn't work.

We now normalize all package data internally so that overrides for ABC, abc, and Abc all map to the same package. In addition we have added information tracking the proper name of bundled packages. This allows us to warn when an override uses the non correct package name, and treat it like the proper name.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.

@robertmaynard robertmaynard added the feature request New feature or request label Oct 24, 2024
@robertmaynard robertmaynard added the non-breaking Introduces a non-breaking change label Oct 24, 2024
@robertmaynard robertmaynard force-pushed the fea/warn_when_override_has_inccorect_case branch from 2292f6f to 74a9498 Compare October 25, 2024 14:47
@robertmaynard robertmaynard marked this pull request as ready for review October 25, 2024 14:47
@robertmaynard robertmaynard requested a review from a team as a code owner October 25, 2024 14:47
@vyasr
Copy link
Contributor

vyasr commented Oct 28, 2024

Do we need to put in some sort of check to avoid accidentally introducing name collisions if there really are two packages that only differ by capitalization? I don't think it's critical but IIUC this PR is going to make such cases collide unconditionally.

@robertmaynard
Copy link
Contributor Author

Do we need to put in some sort of check to avoid accidentally introducing name collisions if there really are two packages that only differ by capitalization? I don't think it's critical but IIUC this PR is going to make such cases collide unconditionally.

Do you mean when the package is listed twice in the defaults.json? The PR does warn if an override has a different case.
Multiple overrides of the same package can't clash as only the first occurence of each package is read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants