-
Notifications
You must be signed in to change notification settings - Fork 809
mod(cinnamon-settings): use fstrings #13074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
note: no logic changes note: some edge cases are still left Signed-off-by: Allen <[email protected]>
|
Hi, this is ok for non-translated strings, but the old format needs to be maintained for anything with gettext ( |
Signed-off-by: Allen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes Python string formatting by converting old-style % formatting and .format() calls to f-strings across the cinnamon-settings codebase. While the intention is good and most conversions are correct, there are 4 critical syntax errors that will cause the code to fail at runtime, and several instances of unnecessarily verbose format specifiers.
Key Issues Found:
- 4 syntax errors that will break functionality (missing commas, missing braces, incorrect quote nesting)
- 11 instances of unnecessary
:dformat specifiers for integers - 2 instances of redundant f-string usage
Reviewed Changes
Copilot reviewed 15 out of 23 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| xlet-settings.py | Converted path construction and print statements to f-strings - looks good |
| cs_user.py | Converted error message to f-string - looks good |
| cs_themes.py | Converted theme path construction to f-strings - syntax error on line 872 (missing comma) |
| cs_startup.py | Converted directory paths and error messages to f-strings - redundant formatting |
| cs_sound.py | Converted markup and error messages to f-strings - syntax error on line 817 (missing braces) |
| cs_power.py | Converted device descriptions and error messages to f-strings - looks good |
| cs_panel.py | Converted debug print statements to f-strings - redundant format specifiers |
| cs_info.py | Converted error messages to f-strings - looks good |
| cs_gestures.py | Converted setting strings to f-strings and removed blank line - looks good |
| cs_extensions.py | Converted directory path to f-string - looks good |
| cs_desklets.py | Converted directory path to f-string - looks good |
| cs_default.py | Converted markup and print statements to f-strings - syntax error on line 262 (missing braces) |
| cs_backgrounds.py | Converted path construction and error messages to f-strings - syntax error on line 780 (quote nesting) |
| cs_applets.py | Converted directory path to f-string - looks good |
| cinnamon-settings.py | Converted timing output and application ID to f-strings - syntax errors and redundant specifiers |
| proxygsettings.py | Converted proxy URL construction to f-strings - redundant format specifiers |
| imtools.py | Converted error message to f-string - looks good |
| capi.py | Converted paths and error messages to f-strings - looks good |
| Spices.py | Converted file paths and error messages to f-strings - looks good |
| KeybindingWidgets.py | Converted error messages to f-strings - looks good |
| JsonSettingsWidgets.py | Converted error messages to f-strings - looks good |
| CinnamonGtkSettings.py | Converted CSS rules and style properties to f-strings - redundant format specifiers |
| ChooserButtonWidgets.py | Converted markup lambda to f-string - looks good |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
files/usr/share/cinnamon/cinnamon-settings/cinnamon-settings.py
Outdated
Show resolved
Hide resolved
files/usr/share/cinnamon/cinnamon-settings/modules/cs_themes.py
Outdated
Show resolved
Hide resolved
files/usr/share/cinnamon/cinnamon-settings/modules/cs_default.py
Outdated
Show resolved
Hide resolved
files/usr/share/cinnamon/cinnamon-settings/modules/cs_backgrounds.py
Outdated
Show resolved
Hide resolved
files/usr/share/cinnamon/cinnamon-settings/cinnamon-settings.py
Outdated
Show resolved
Hide resolved
files/usr/share/cinnamon/cinnamon-settings/cinnamon-settings.py
Outdated
Show resolved
Hide resolved
|
For the record, I had copilot do a review not because I value its coding knowledge and prowess (I don't, really) but it helps me when I do large sets of changes (especially repetitive things, refactoring) to catch careless detail things. It looks like it found a number of things (I would have skimmed right past!). You should be able to get a re-review from it after you fix things (upper right corner of the page). If it's not offering you that option, let me know I'll trigger it. |
Signed-off-by: Allen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 23 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
files/usr/share/cinnamon/cinnamon-settings/modules/cs_themes.py
Outdated
Show resolved
Hide resolved
linuxmint#13074 (review) Co-authored-by: Copilot <[email protected]>
Modify files under /cinnamon-settings/ to use fstrings.
Did some tests locally but more tests are welcome.
Note:
Versions: