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: allow duplicate registry values #7988

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

maribethb
Copy link
Contributor

The basics

The details

Resolves

Fixes #7924

Proposed Changes

Does not throw an error if you attempt to register the same object twice (under the same type and case-insensitive name)

Reason for Changes

This is similar to #7983. The difference in behavior is subtle:

In this PR: If you try to register the same TestObject twice, first under the name test and second under the name Test, then we'll allow you to register the object without throwing an error and we'll also update the cased name to be Test, which only matters if you later call getAllItems(type, true) where we give you the most-recently used cased version of the name. This is consistent with how it works if you pass a different object, same name, and pass the opt_allowOverrides parameter: we'll update the case of the stored name.

In #7983: If you try to register the same TestObject twice, first under the name test and second under the name Test, then we'll allow you to register the object without throwing an error. But we won't update the most-recently-cased version of the name. This is inconsistent with how it works if you pass a different object, same name, and pass the opt_allowOverrides. It's also a behavior change where previously, you could register the same object and intentionally change the case by passing the opt_allowOverrides flag.

I think maintaining consistency with how the overrides flag works is better, and it makes it easier to explain the "last used case" part of the registry that keeps track of the case-sensitive name you registered an item under. That's why I think this PR should be merged instead of #7983 (or #7982)

Test Coverage

Added unit test for this case.

Documentation

Maybe? We don't really explain the registry anywhere. The most likely place this gets explained is in the documentation about how installing fields/blocks plugins works.

Additional Information

See #7924 and #7987 for related discussion.

@maribethb maribethb requested a review from a team as a code owner April 1, 2024 21:02
@github-actions github-actions bot added the PR: feature Adds a feature label Apr 1, 2024
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

I agree with your logic in choosing to be consistent with the behaviour for new names with opt_allowOverrides

@maribethb maribethb merged commit 6767717 into google:develop Apr 1, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants