-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
fix: correct Linux distro version in systeminfo prompt #239
Conversation
Closes #238 |
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.
❌ Changes requested. Reviewed everything up to f57e55a in 56 seconds
More details
- Looked at
32
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_W5IyvzV3QkjJzNxM
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
gptme/prompts.py
Outdated
@@ -247,6 +247,18 @@ def get_system_distro() -> str: | |||
return "Linux" | |||
|
|||
|
|||
def get_system_distro_version() -> str: | |||
"""Get the system distribution version.""" | |||
regex = re.compile(r"^BUILD_ID=\"?([^\"]+)\"?") |
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.
The regex pattern should match VERSION_ID
instead of BUILD_ID
to correctly extract the system distribution version from /etc/os-release
.
regex = re.compile(r"^BUILD_ID=\"?([^\"]+)\"?") | |
regex = re.compile(r"^VERSION_ID=\"?([^\"]+)\"?") |
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.
On arch it's BUILD_ID. But even co-pilot suggested the same thing. Let me check on fedora.
gptme/prompts.py
Outdated
@@ -247,6 +247,18 @@ def get_system_distro() -> str: | |||
return "Linux" | |||
|
|||
|
|||
def get_system_distro_version() -> str: |
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.
I would merge this with get_system_distro
and make it return a tuple[str, str]
Just learned about |
Important
Corrects Linux OS version retrieval in
prompt_systeminfo()
by usingget_system_distro_version()
instead ofplatform.uname().release
.prompt_systeminfo()
by replacingplatform.uname().release
withget_system_distro_version()
.get_system_distro_version()
to extract the Linux distribution version from/etc/os-release
using regex.This description was created by for f57e55a. It will automatically update as commits are pushed.