Fixup FolderId usage and change log#12986
Conversation
|
ccing @feerrenrut as an additional reviewer what are your thoughts on the discussion of casing of Enum names and members? #12943 (comment) |
This comment has been minimized.
This comment has been minimized.
| """Wrapper for `SHGetKnownFolderPath` which caches the results | ||
| to avoid calling the win32 function unnecessarily.""" | ||
| guid = comtypes.GUID(folderGuid) | ||
| guid = comtypes.GUID(folderGuid.value) |
There was a problem hiding this comment.
While this certainly improves readability it also makes this much less usable for consumers of SHGetKnownFolderPath i.e. add-on developers.
Since our FolderId contains only guids of folders which we're actually using in NVDA if someone wants to retrieve path of a different folder (Microsoft lists a lot of them) they would need to implement SHGetKnownFolderPath themselves. Adding all known guids to our enum is also not a solution since list of known folder can be extended by program installers
I'd like to propose modifying this function so that it accepts union[FolderId, str] as its first parameter and then either accesses the value or just creates a comtypes.GUID from the string provided. That way we would offer similar level of flexibility as we used to with our wrapper for SHGetFolderPath in 2021.3.
There was a problem hiding this comment.
This sounds reasonable given the requirements.
There was a problem hiding this comment.
good point, this should be included now.
|
While camel case (actually "Pascal Case" because of the leading capital) is used for class names (including Enum classes) such as |
good catch - fixed |
Link to issue number:
Fixup of #12943
Summary of the issue:
Changelog entries were missing for #12943
UpperCamelCase is the standard for class names, including enums, however
FOLDERIDwas used as the casing for the enum, which mirrors the windows constants.There is a similar issue for the enum members (which used UpperCamelCase instead of CAP_SNAKE_CASE).
If we want to change our practice to be consistent with what an Enum represents, we should update
codingStandards.mdAlso the enum type was not being fully leveraged by
SHGetKnownFolderPath, requiring.valueto be used unnecessarily when calling the function.Description of how this pull request fixes the issue:
Updates the casing of the enum and it's members. Updates the change log, and fixes up some earlier entries.
Testing strategy:
None
Known issues with pull request:
None
Change log entries:
See diff
Code Review Checklist: