add clang and ubuntu versions warnings#502
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the clang version requirement from 16 to 21 and adds a new Ubuntu version warning to alert users when they're not running on Ubuntu 22.04 or 24.04.
Key changes:
- Increased required clang version from 16 to 21 with updated documentation URL
- Refactored
get_clang_major_version()function from mytonctrl.py to utils.py for better code organization - Added new
get_os_version()utility function andcheck_ubuntu_version()warning function - Added Ubuntu version warning translation strings in English, Russian, and Chinese
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| mytonctrl/mytonctrl.py | Updated CLANG_VERSION_REQUIRED constant to 21, added check_ubuntu_version function, refactored get_clang_major_version to utils.py, updated warning URL |
| mytonctrl/utils.py | Added get_clang_major_version and get_os_version utility functions |
| mytonctrl/resources/translate.json | Added ubuntu_version_warning translation strings |
| tests/integration/test_basic_commands.py | Updated test mocks to use clang version 21 instead of 16 |
| tests/integration/test_preup.py | New test file for testing Ubuntu version warnings |
| tests/conftest.py | Renamed MyMyPyConsole to TestMyPyConsole and added run_pre_up method for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/integration/test_preup.py
Outdated
| def test_warnings(cli, monkeypatch, mocker: MockerFixture): | ||
| monkeypatch.setattr(mytonctrl, 'CheckMytonctrlUpdate', lambda *_: None) | ||
| monkeypatch.setattr(mytonctrl, 'check_installer_user', lambda *_: None) | ||
| monkeypatch.setattr(mytonctrl, 'check_installer_user', lambda *_: None) |
There was a problem hiding this comment.
The function 'check_installer_user' is being monkeypatched twice with the same lambda. This is redundant - one of these lines should be removed.
| monkeypatch.setattr(mytonctrl, 'check_installer_user', lambda *_: None) |
tests/integration/test_preup.py
Outdated
| VERSION="20.04.4 LTS (Jammy Jellyfish)" | ||
| VERSION_CODENAME=jammy |
There was a problem hiding this comment.
The VERSION field in the mock data shows "20.04.4 LTS (Jammy Jellyfish)" but Jammy Jellyfish is the codename for Ubuntu 22.04, not 20.04. Ubuntu 20.04 is "Focal Fossa". This should be corrected to maintain accurate test data.
| VERSION="20.04.4 LTS (Jammy Jellyfish)" | |
| VERSION_CODENAME=jammy | |
| VERSION="20.04.4 LTS (Focal Fossa)" | |
| VERSION_CODENAME=focal |
tests/integration/test_preup.py
Outdated
| VERSION="20.04.4 LTS (Jammy Jellyfish)" | ||
| VERSION_CODENAME=jammy |
There was a problem hiding this comment.
Similarly, the VERSION_CODENAME shows "jammy" but should be "focal" for Ubuntu 20.04.
| VERSION="20.04.4 LTS (Jammy Jellyfish)" | |
| VERSION_CODENAME=jammy | |
| VERSION="20.04.4 LTS (Focal Fossa)" | |
| VERSION_CODENAME=focal |
| if distro == 'ubuntu': | ||
| if ver not in ['22.04', '24.04']: | ||
| warning = local.translate("ubuntu_version_warning").format(ver) | ||
| print_warning(local, warning) |
There was a problem hiding this comment.
The 'check_ubuntu_version' function is missing an #end define comment that is used consistently throughout the rest of the file. For consistency, it should be added.
| print_warning(local, warning) | |
| print_warning(local, warning) | |
| #end define |
| local.try_function(check_slashed, args=[local, ton]) | ||
| #end define | ||
|
|
||
| local.try_function(check_ubuntu_version, args=[local]) |
There was a problem hiding this comment.
The 'warnings' function is missing an #end define comment that is used consistently throughout the rest of the file. For consistency, it should be added.
| local.try_function(check_ubuntu_version, args=[local]) | |
| local.try_function(check_ubuntu_version, args=[local]) | |
| #end define |
tests/integration/test_preup.py
Outdated
| output = cli.run_pre_up() | ||
| assert 'Ubuntu' not in output | ||
|
|
||
| # todo: other warnings No newline at end of file |
There was a problem hiding this comment.
The test file is named 'test_preup.py' and tests the warnings function, but it only tests Ubuntu version warnings. The PR title mentions "add clang and ubuntu versions warnings" but there are no tests for clang version warnings. Consider adding test coverage for clang version checks to match the PR's stated purpose.
mytonctrl/resources/translate.json
Outdated
| "ubuntu_version_warning": { | ||
| "en": "{{red}}Ubuntu version must be 22.04 or 24.04. Found {0}. {{endc}}", | ||
| "ru": "{{red}}Версия Ubuntu должна быть 22.04 или 24.04. Найдена {0}. {{endc}}", | ||
| "zh_TW": "{{red}}Ubuntu 版本必须是 22.04 或 24.04。找到 {0}。{{endc}}" |
There was a problem hiding this comment.
The zh_TW (Traditional Chinese) translation contains simplified Chinese characters "必须" which should be "必須" (traditional). The word "找到" should also be verified for consistency with Traditional Chinese standards.
| "zh_TW": "{{red}}Ubuntu 版本必须是 22.04 或 24.04。找到 {0}。{{endc}}" | |
| "zh_TW": "{{red}}Ubuntu 版本必須是 22.04 或 24.04。找到 {0}。{{endc}}" |
tests/integration/test_preup.py
Outdated
| import builtins | ||
|
|
There was a problem hiding this comment.
Import of 'builtins' is not used.
| import builtins |
add ubuntu warning test
1 participant
No description provided.