Modernise speech dict handler with enums and type hints#19430
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the speech dictionary handler module by introducing type hints, enums, and dataclasses as preparation for future add-on support. The refactoring aims to improve code maintainability and type safety without changing user-facing behavior.
Changes:
- Introduced
EntryTypeandDictionaryTypeenums to replace integer and string constants - Converted
SpeechDictEntryfrom a class to a dataclass and moved to newtypesmodule - Added comprehensive type hints throughout the module
- Replaced deprecated
codecs.openwith built-inopen - Established deprecation paths for moved symbols using
MovedSymbolhelper
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/changes.md | Documents deprecations for moved constants and types |
| source/speechDictHandler/types.py | New module containing type definitions (enums, dataclass, SpeechDict class) |
| source/speechDictHandler/dictFormatUpgrade.py | Modernized with f-strings |
| source/speechDictHandler/init.py | Refactored to use new types and deprecation handlers |
| source/gui/speechDict.py | Updated to use EntryType enum instead of integer constants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| log.exception( | ||
| 'Dictionary ("%s") entry invalid for "%s" error raised: "%s"' | ||
| % (fileName, line, e), | ||
| ) | ||
| comment = "" | ||
| else: | ||
| log.warning("can't parse line '%s'" % line) | ||
| log.debug("%d loaded records." % len(self)) |
There was a problem hiding this comment.
can these be turned to fstrings?
There was a problem hiding this comment.
Use f strings in logging is considered bad practice, just as bad as the current statements are. Logging has build in support for delayed formatting. I'll change to something like
| log.exception( | |
| 'Dictionary ("%s") entry invalid for "%s" error raised: "%s"' | |
| % (fileName, line, e), | |
| ) | |
| comment = "" | |
| else: | |
| log.warning("can't parse line '%s'" % line) | |
| log.debug("%d loaded records." % len(self)) | |
| log.exception( | |
| 'Dictionary ("%s") entry invalid for "%s" error raised: "%s"', | |
| fileName, line, e, | |
| ) | |
| comment = "" | |
| else: | |
| log.warning("can't parse line '%s'", line) | |
| log.debug("%d loaded records.", len(self)) |
There was a problem hiding this comment.
I agree that this format might be better, but our coding standards has historically between to use them, despite performance, unless there is a serious performance impact.
Thoughts @SaschaCowley? We probably need to document some form of preference in our coding standards.
There was a problem hiding this comment.
@SaschaCowley - review requested in regards to this formatting question
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
seanbudd
left a comment
There was a problem hiding this comment.
Thanks @LeonarddeR - I'm going to merge this for now to avoid bikeshedding around f-strings. It would be good for us to come up with a policy in codingStandards for the future though
Summary of the issue:
The speech dict handler didn't have type hints and could use some modernization here and there.
Description of user facing changes:
None
Description of developer facing changes:
See deprecations in changelog
Description of development approach:
Change ENTRY_TYPE_* integer constants into an enum
Use an str enum for dictionary types
Use a dataclass for DictionaryEntry
Move some types internal to dictionary processing to its own types module to split up the package
Swap codecs.open for open, since codecs.open is deprecated in Python 3.14
Link to issue number:
None, but necessary preparations for #17468
Summary of the issue:
The speech dict handler didn't have type hints and could use some modernization here and there.
Description of user facing changes:
None
Description of developer facing changes:
See deprecations in changelog
Description of development approach:
Testing strategy:
Tested that dictionaries still work and can be read/written to.
Known issues with pull request:
None
Code Review Checklist: