OptionList safe duplicate check
#3459
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Simply put: until now the OptionList was adding the new options and then checking if the ID was a duplicate. If some code was to then catch DuplicateID and treat it as a pass this would then have an unintended side-effect that the duplicate had been added anyway. The ultimate effect of this being that once there was an attempt to add a duplicate, nothing more could be added to that OptionList. Fixes Textualize#3455.
This way there's less chance of a duplicate report of a duplicate.
TomJGooding
reviewed
Oct 4, 2023
In conversation we've decided that it's not that necessary to list all of the IDs that are duplicates; with this in mind we can whittle down the work being done to look for duplicates. Co-authored-by: Will McGugan <[email protected]>
Because apparently I can't type English.
Likely doesn't make a whole heap of difference, but the {} vs the [] was a
typo and really conceptually it is a list of new options with IDs.
willmcgugan
approved these changes
Oct 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simply put: the duplicate check inside
OptionListwas happening after a new option was added to the internal structures. This was almost fine back when there was only a method for adding a single option, and even then it was only fine if aDuplicateIDexception was treated as a "just give up using thisOptionListsignal". Or put another way: attempting to add a duplicate ID had unintended side-effects, making every subsequent attempt to add an option appear to be the creation of a duplicate.This PR moves to a preflight check for duplicate IDs.
Fixes #3455.