Fix uses of newValue
#16846
Fix uses of newValue
#16846
Conversation
colin-grant-work
force-pushed
the
bugfix/new-value
branch
from
dbf52e7 to
a0f9d0c
Compare
ndoschek
left a comment
ndoschek
left a comment
There was a problem hiding this comment.
Thanks a lot, Colin, for this great improvement! Glad we could finally resolve this newValue trap for developers now.
I had a look and tested several of the updated settings, changing and resetting them works as expected 🎉
I only have a few very minor comments. It would be great if you could have a quick look when you get a chance. TIA!
packages/ai-ollama/src/browser/ollama-frontend-application-contribution.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
For this setting there seems to be an issue (for both browser and electron though), when I change the setting to compact and back/reset to classic, the menu does not show again as expected, but only shows a ... button.
But this was not introduced with this PR, so I'm going to open a follow up for this.
packages/terminal-manager/src/browser/terminal-manager-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
| toDispose.dispose(); | ||
| assert(results.every(setting => setting.status === 'fulfilled'), 'All promises should have resolved rather than rejected.'); | ||
| assert.deepEqual(actualValues, eventValues, 'The event should reflect the current state of the service.'); | ||
| assert.deepEqual([channelPref, hoverPref, searchPref], eventKeys, 'The event should contain the changed preference names.'); |
There was a problem hiding this comment.
One more thing I just noticed: we seem to be hitting issues with this equality check in the CI runs on Node 20. Maybe we could use a less strict comparison for the array?
This reverts commit d9a25f5.
Co-authored-by: Nina Doschek <ndoschek@eclipsesource.com>
colin-grant-work
force-pushed
the
bugfix/new-value
branch
from
1879c49 to
eeee50b
Compare
colin-grant-work
force-pushed
the
bugfix/new-value
branch
from
eeee50b to
58169d6
Compare
What it does
How to test
Fixes #16645
Fixes #16821
Offers two alternatives:
newValuemeaningful. Then it would be (kind of) safe to use. I think that would be nice, but it would be a somewhat large change to the API. We're very clear that you shouldn't usenewValueas new value (makes one wonder why we include it), and we do provide ascopein which the change occurred, and giving a different value might be misleading.newValuethe result of getting the new value. This is the safest.Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers