when upgrading config from schema 20 to 21, gracefully handle when a key or value does not exist E.g. for a config profile.#19519
Conversation
…key or value does not exist E.g. for a config profile.
There was a problem hiding this comment.
Pull request overview
Fixes profile schema upgrade (20 → 21) to avoid exceptions when profiles don’t contain expected speech/synth keys, addressing a regression introduced by PR #19432.
Changes:
- Update
upgradeConfigFrom_20_to_21to use.get()lookups instead of direct key access for[speech]/synth. - Add debug logging and early return when
[speech]is absent. - Continue migrating legacy
sapi4/sapi5subsections tosapi4_32/sapi5_32.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Redirect old sapi4 and sapi5 config to 32 bit versions.""" | ||
| synth = profile["speech"]["synth"] | ||
| speechConf = profile.get("speech") | ||
| if not speechConf: |
There was a problem hiding this comment.
speechConf = profile.get("speech") followed by if not speechConf: treats an existing but empty [speech] section the same as a missing section, and the log message becomes inaccurate (it may log “does not have a speech key” even when the key exists). Consider checking explicitly with if "speech" not in profile: (or if speechConf is None:) and adjust the message accordingly so it only fires when the section truly is absent.
| if not speechConf: | |
| if "speech" not in profile: |
| sapi4Conf = speechConf.get("sapi4") | ||
| if sapi4Conf: | ||
| profile["speech"]["sapi4_32"] = sapi4Conf | ||
| del profile["speech"]["sapi4"] | ||
| sapi5Conf = profile["speech"].get("sapi5") | ||
| speechConf["sapi4_32"] = sapi4Conf | ||
| del speechConf["sapi4"] | ||
| log.debug("Moved old sapi4 configuration values to sapi4_32") | ||
| sapi5Conf = speechConf.get("sapi5") | ||
| if sapi5Conf: | ||
| profile["speech"]["sapi5_32"] = sapi5Conf | ||
| del profile["speech"]["sapi5"] | ||
| speechConf["sapi5_32"] = sapi5Conf | ||
| del speechConf["sapi5"] |
There was a problem hiding this comment.
if sapi4Conf: / if sapi5Conf: will skip migration when the [speech][sapi4] or [speech][sapi5] section exists but is empty (ConfigObj sections are dict-like and evaluate false when empty). If the goal is to remove legacy keys whenever they exist, consider checking for key presence (e.g. if "sapi4" in speechConf: / if sapi4Conf is not None:) so even empty sections get renamed/deleted.
SaschaCowley
left a comment
There was a problem hiding this comment.
Looks good. Just a small clarification to the log message when there is no speech section in the config. Let me know if you think it's overkill and I'll approve without it though.
Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
Link to issue number:
Addresses an issue introduced by pr #19432
Summary of the issue:
With the merging of pr #19432 which adds SAPI 32 bit support, An error is produced when switching to config profiles that do not have particular synth keys already set.
Description of user facing changes:
No error is produced and profiles load correctly.
Description of developer facing changes:
Description of development approach:
config.profileUpgradeSteps.upgradeConfigFrom_20_to_21: Fetch existing keys withgetrather than key lookup.Testing strategy:
synth = sapi4, and both sapi4 and sapi5 sections in speech, ensured that the synth value was changed to sapi4_32, and that the sapi4 section was renamed to sapi4_32, and that the sapi5 section was renamed to sapi5_32 when running NvDA, and that NVDA successfully used the sapi4_32 synth.Known issues with pull request:
None known.
Code Review Checklist: