Only offer latest release if it is semver-newer than current #715
Conversation
WalkthroughThe pull request introduces several changes across multiple files. Key modifications include updates to dependency management in Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files not reviewed due to no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- .gitignore (1 hunks)
- requirements.build.txt (1 hunks)
- requirements.txt (0 hunks)
- src/main/python/main/ayab/utils.py (1 hunks)
- src/main/python/main/ayab/version_checker.py (1 hunks)
- src/main/python/main/main.py (1 hunks)
- src/main/resources/base/ayab/package_version (0 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- requirements.txt
- src/main/resources/base/ayab/package_version
🔇 Additional comments (9)
requirements.build.txt (1)
11-11: LGTM! Changes align with PR objectives.The replacement of
requests==2.31.0withsemver==3.0.2is in line with the PR objectives:
- Removing the
requestsdependency- Implementing a semantic versioning check
These changes should help address the issue of incorrect update prompts and improve version comparison.
To ensure the complete implementation of the PR objectives, please run the following script to verify the removal of
requestsand the use of Qt's HTTP client:This script will help confirm that:
- The
requestslibrary is no longer used in the codebase.- Qt's network module and HTTP client (QNetworkAccessManager) are being used as a replacement.
Please review the results and ensure that the appropriate changes have been made throughout the codebase.
✅ Verification successful
Verified! Changes align with PR objectives.
- The
requestsdependency has been successfully removed.QNetworkAccessManagerfrom Qt's network module is implemented as intended.These changes ensure the removal of the
requestslibrary and the adoption of Qt's built-in HTTP client, addressing the issue of incorrect update prompts and enhancing version comparison.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify removal of requests and implementation of Qt's HTTP client # Test 1: Check if 'requests' is still imported anywhere echo "Checking for 'requests' imports:" rg 'import requests' --type python # Test 2: Look for Qt's network module usage echo "Checking for Qt's network module usage:" rg 'from PySide6 import QtNetwork' --type python # Test 3: Look for QNetworkAccessManager usage (Qt's HTTP client) echo "Checking for QNetworkAccessManager usage:" rg 'QNetworkAccessManager' --type pythonLength of output: 487
Script:
#!/bin/bash # Description: Verify removal of requests and implementation of Qt's HTTP client using file globs # Test 1: Check if 'requests' is still imported anywhere echo "Checking for 'requests' imports:" rg 'import requests' --glob '*.py' # Test 2: Look for Qt's network module usage echo "Checking for Qt's network module usage:" rg 'from PySide6 import QtNetwork' --glob '*.py' # Test 3: Look for QNetworkAccessManager usage (Qt's HTTP client) echo "Checking for QNetworkAccessManager usage:" rg 'QNetworkAccessManager' --glob '*.py'Length of output: 720
.gitignore (1)
11-11: Approved: Clarification of CI-generated file commentThe change from "# File is changed by CI" to "# File is generated by CI" provides a more accurate description of how the
package_versionfile is handled in the CI process. This aligns well with the PR's focus on improving version management and clarity.src/main/python/main/ayab/utils.py (3)
Line range hint
1-116: Summary of changes in utils.pyThe modifications to this file align with the PR objectives:
- The
package_versionfunction has been updated to use a resource file and handle errors, which improves maintainability and robustness.- The
latest_versionfunction has been removed, likely as part of the effort to remove therequestsdependency and improve the update check mechanism.These changes contribute to the overall goal of improving the version checking system. However, ensure that the suggestions for improving the
package_versionfunction are addressed, and verify that the update checking functionality has been properly implemented elsewhere in the codebase.
Line range hint
1-116: Verify replacement oflatest_versionfunctionalityThe
latest_versionfunction has been removed from this file. While this aligns with the PR objectives to improve the update check mechanism and remove therequestsdependency, we need to ensure that this functionality has been adequately replaced.Please run the following verification to confirm that the update checking functionality has been implemented elsewhere:
#!/bin/bash # Search for new update checking implementation rg --type python -A 10 "def.*version.*check|class.*version.*check"This will help us locate any new classes or methods that have been introduced to handle version checking, ensuring that we haven't lost this important functionality.
109-116: 🛠️ Refactor suggestionImprove error handling and type safety in
package_versionfunctionThe refactored
package_versionfunction is an improvement over the previous version, but there are a few areas that could be enhanced:
- Consider using a more specific type for
app_contextinstead ofAnyto improve type safety.- Add logging for the FileNotFoundError to aid in debugging.
- Consider using a more distinctive fake version number for local development.
Here's a suggested improvement:
from typing import Protocol import logging class AppContext(Protocol): def get_resource(self, path: str) -> str: ... def package_version(app_context: AppContext) -> str: try: filename_version = app_context.get_resource("ayab/package_version") with open(filename_version) as version_file: return version_file.read().strip() except FileNotFoundError: logging.warning("Version file not found. Using development version.") # Use a distinctive version number for local development return "0.0.0-dev"This implementation:
- Uses a Protocol for better type hinting of
app_context.- Adds logging for the FileNotFoundError.
- Uses a more distinctive version number for local development.
To ensure that this change doesn't break existing functionality, please run the following verification:
This will help us confirm that all calls to
package_versionare compatible with the new signature.✅ Verification successful
package_versionfunction usage verified and correctAll instances of the
package_versionfunction are correctly passing a validapp_context. Specifically:
- In
main.py,selfis an instance ofAppContext, which inherits fromApplicationContextand possesses theget_resourcemethod.- In
about.py,parent.app_contextis appropriately passed, ensuring type compatibility.No further actions are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of package_version function rg --type python -A 5 "package_version\("Length of output: 99
Script:
#!/bin/bash # Verify the usage of package_version function in Python files rg -g "*.py" -A 5 "package_version\("Length of output: 1409
Script:
#!/bin/bash # Extract package_version function calls with their arguments rg -g "*.py" "package_version\(" --prettyLength of output: 323
Script:
#!/bin/bash # Extract context around line 42 in main.py to verify the structure of 'self' rg -C 5 "^\s*def " src/main/python/main/main.py | rg -B 10 -A 10 "package_version("Length of output: 168
Script:
#!/bin/bash # Extract context around the package_version function call in main.py to verify the structure of 'self' rg -C 5 "^\s*def " src/main/python/main/main.py | rg -B 10 -A 10 "package_version\("Length of output: 902
src/main/python/main/main.py (4)
17-17: ImportingVersionCheckerin type-checking blockThe import statement correctly adds
VersionCheckerto theTYPE_CHECKINGblock, ensuring that type hints are properly resolved during static analysis without affecting runtime performance.
25-25: ImportingVersionCheckerin the main import blockThe addition of
VersionCheckerto the main import block ensures that the class is available at runtime, which is necessary for the application's functionality.
29-29: Handling alternate import paths forVersionCheckerIncluding
VersionCheckerin the exception block accommodates different execution environments where the package structure may vary. This maintains compatibility when the module is imported differently, such as during development or packaging.
34-34: Adding_version_checkerattribute with type hintDefining
_version_checker: VersionCheckerprovides a type hint for the attribute, enhancing code readability and enabling better static type checking.
* Move version checking logic to new VersionChecker class. * Use Qt network classes instead of pulling in requests. * Use an async request to avoid blocking the main thread while the check is being done. * Display current version in "new version available" pop-up.
This lets the app run without a `package_version` file, so it can be removed from source control which is a bit cleaner than overwriting it during the build.
This is to avoid suggesting 1.0.0 (assuming it is the latest non-prerelease) to people running e.g. 1.1.0-beta1. Ideally we'd offer 1.1.0-beta2 to users of 1.1.0-beta1 while keeping 1.0.0 users on 1.0.0, but this would require the ability to ask GitHub for the latest prerelease which their API doesn't make easy unfortunately.
jonathanperret
force-pushed
the
647-mac-outdated-new-version-dialog-appears-on-launch
branch
from
06bea53 to
879c776
Compare
|
Agreed. I saw #484 but wasn't sure whether it was in scope for 1.0. |
jonathanperret
deleted the
647-mac-outdated-new-version-dialog-appears-on-launch
branch
Problem
Fixes #647.
Proposed solution
In this PR the update check is changed from an equality comparison (suggest an update if the current version is different from the latest) to a semver check, so that:
1.0.0and1.1.0is the latest release, you'll be prompted to download it (this already worked);1.1.0-beta1and1.1.0is the latest release, you will be prompted (already worked too);1.1.0-beta1and1.0.0is the latest release, you won't be prompted (differs from current behavior).So the main difference is that beta testers won't be prompted to revert to a stable release.
Ideally we'd suggest
1.1.0-beta2to users of1.1.0-beta1while keeping1.0.0users on1.0.0(in other words, proper release channels), but this would require the ability to ask GitHub for the latest prerelease which their API doesn't make easy unfortunately (the only way I can see is to download the whole release list, which can be hundreds of kilobytes and possibly require multiple requests — note that you can't even rely on releases being listed in any order, see https://github.com/orgs/community/discussions/8226). Alternatively, we could publish a JSON file with a list of current versions, but this is more work.This PR also changes the update check to run in the background so that it does not slow down application startup, and removes the
requestsdependency in favor of Qt's built-in HTTP client.Summary by CodeRabbit
New Features
VersionCheckerclass for checking the latest software version from GitHub.Bug Fixes
Chores
requestswithsemverfor better version management.requestsand added forPillow.Documentation
.gitignorefile regarding file generation by CI.PACKAGE_VERSIONfile, affecting version management.