-
Notifications
You must be signed in to change notification settings - Fork 12
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
Reworked update station files, version upgrade and traslation #57
base: master
Are you sure you want to change the base?
Conversation
I remove the force upgrade everything this was casing self-heal to loop. This fix is related to ghostbsd/issues#29
Reviewer's Guide by SourceryThis PR extensively reworks the update station implementation, focusing on improving translation handling and code organization. The changes include a new translation management system, restructured package layout, and enhanced UI components with better error handling and user notifications. Class diagram for translation management systemclassDiagram
class UpdateTranslationsCommand {
+run()
+initialize_options()
+finalize_options()
}
class CreateTranslationCommand {
+run()
+initialize_options()
+finalize_options()
+locale
}
Command <|-- UpdateTranslationsCommand
Command <|-- CreateTranslationCommand
note for UpdateTranslationsCommand "Handles extraction and updating of translation files"
note for CreateTranslationCommand "Creates new translation files for specified locales"
Class diagram for update station UI componentsclassDiagram
class UpdateWindow {
+delete_event()
+start_update()
+if_backup()
+create_bbox()
}
class InstallUpdate {
+close_application()
+read_output()
+stop_tread()
}
class StartCheckUpdate {
+close_application()
+check_for_update()
+update_progress()
}
class UpdateNotifier {
+notify()
+on_activated()
}
class TrayIcon {
+tray_icon()
+nm_menu()
+left_click()
+icon_clicked()
}
UpdateWindow <|-- InstallUpdate
UpdateWindow <|-- StartCheckUpdate
UpdateNotifier <|-- TrayIcon
note for UpdateWindow "Main window for update management"
note for InstallUpdate "Displays update progress"
note for StartCheckUpdate "Checks for available updates"
note for UpdateNotifier "Handles update notifications"
note for TrayIcon "Manages system tray icon for updates"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ericbsd - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add tests to address the upgrade loop issue mentioned in the PR description before merging
- The README should include more detailed documentation about the new translation workflow and commands
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 3 issues found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
:param fraction: The fraction to add. | ||
:param text: The text to display. | ||
""" | ||
new_val = progress.get_fraction() + fraction |
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.
suggestion: Add bounds checking to prevent progress bar fraction from exceeding valid range
The progress bar fraction should be clamped between 0.0 and 1.0 to prevent invalid states. Consider using: new_val = min(1.0, max(0.0, progress.get_fraction() + fraction))
new_val = progress.get_fraction() + fraction | |
new_val = min(1.0, max(0.0, progress.get_fraction() + fraction)) |
self.notification = None | ||
Notify.init('Test') | ||
self.msg = _("Software updates are now available.") | ||
self.timeout = 10000 # 10 seconds |
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.
issue: Unused timeout variable in notification setup
The timeout value is defined but never used. Either remove it or pass it to the notification with set_timeout().
GhostBSD update manager. | ||
|
||
|
||
## Create up |
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.
suggestion (documentation): Section title 'Create up' is unclear and doesn't reflect the content about translations
Consider using a more descriptive title like 'Translation Setup' or 'Managing Translations'
## Create up | |
## Managing Translations |
|
||
|
||
## Create up | ||
To create translation file. |
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.
nitpick (documentation): Missing article before 'translation file'
Should be 'To create a translation file.'
./setup.py create_translation --locale=fr | ||
``` | ||
|
||
To updated translation files |
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.
nitpick (documentation): Grammatical error in command description
Should be 'To update translation files'
backup_checkbox.set_active(True) | ||
backup_checkbox.set_sensitive(True) | ||
Data.backup = True | ||
else: | ||
backup_checkbox.set_active(False) | ||
backup_checkbox.set_sensitive(False) | ||
Data.backup = False |
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.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method
)
i_num = 0 | ||
ri_num = 0 | ||
if bool(Data.packages_dictionary['remove']): | ||
r_num = len(Data.packages_dictionary['remove']) |
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.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method
)
self.thr = threading.Thread(target=self.read_output, args=[self.pbar], daemon=True) | ||
self.thr.start() | ||
|
||
def read_output(self, progress): |
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.
issue (code-quality): We've found these issues:
- Extract code out into method (
extract-method
) - Use
with
when opening file to ensure closure [×6] (ensure-file-closed
) - Low code quality found in InstallUpdate.read_output - 2% (
low-code-quality
)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
""" | ||
if updating(): | ||
unlock_update_station() | ||
if fail is True: |
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.
issue (code-quality): We've found these issues:
- Merge else clause's nested if statement into elif (
merge-else-if-into-elif
) - Simplify comparison to boolean [×3] (
simplify-boolean-comparison
) - Move assignment closer to its usage within a block [×2] (
move-assign-in-block
)
GLib.idle_add(self.update_progress, progress, | ||
_('The repository is online')) | ||
sleep(1) | ||
if repository_is_syncing() is True: |
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.
issue (code-quality): We've found these issues:
- Merge else clause's nested if statement into elif (
merge-else-if-into-elif
) - Use named expression to simplify assignment and conditional (
use-named-expression
) - Remove redundant conditional (
remove-redundant-if
)
This is the first part for ghostbsd/issues#29
I have reworked extensively, including how the PO and files are created.
More tests are needed to fix the upgrade loop.