Simplify CreateProfile, fix bug filling list of profiles#6499
Simplify CreateProfile, fix bug filling list of profiles#6499evankanderson wants to merge 1 commit into
Conversation
ca7baf7 to
9afd833
Compare
9afd833 to
3b97ab4
Compare
krrish175-byte
left a comment
There was a problem hiding this comment.
LGTM! Clean fix for a real bug.
The old code allocated existingProfileNames as an empty slice and never populated it, so DeriveProfileNameFromDisplayName always saw zero existing names and the collision check was a no-op.
Switching from strings.Contains(strings.Join(...)) to a direct map lookup also fixes the false-positive substring matching problem (e.g. a derived name foo would have incorrectly collided with an existing profile named foobar).
The test string corrections (_when -> _whe) make sense since the 63-char limit truncates one character earlier than the old tests assumed.
|
LGTM. The map lookup is correct and eliminates both the unpopulated slice bug and the substring false-positive. The test corrections for the 63-char truncation boundary make sense — the old tests were asserting wrong behavior |
Summary
I was chasing a different bug, and noticed that CreateProfile was attempting to name unnamed profiles uniquely, but actually discarded the map of profile names. Testing for membership in a map will also be faster (and more correct) than repeated
string.Contains. It turns out that some of the tests were also incorrect (wrong number of characters chopped), so fixed that as well.Testing
Verified via unit tests.