Skip to content

Simplify os_description trim; rebase on main#75

Merged
dominicletz merged 1 commit into
mainfrom
cursor/pull-request-74-review-9f20
Jul 21, 2026
Merged

Simplify os_description trim; rebase on main#75
dominicletz merged 1 commit into
mainfrom
cursor/pull-request-74-review-9f20

Conversation

@dominicletz

@dominicletz dominicletz commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Rebased onto main (PR Add Platform.System.os_description/0 #74 already merged)
  • Simplified nested case in Desktop.Platform.System.os_description/0 to if raw do … end (Gemini feedback)
  • Removed no-op Process.delete/1 from on_exit in platform_system_test.exs (cleanup runs in a separate process)

Test plan

  • mix test test/desktop/platform_system_test.exs test/desktop/backend/json_test.exs test/desktop/backend/browser_test.exs test/desktop/regression/beam_wx_calls_test.exs --exclude wx
  • mix test.guard
  • xvfb-run -a mix test test/desktop/backend/wx_test.exs --only wx
Open in Web Open in Cursor 

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces Desktop.Platform.System.os_description/0 as a backend-safe replacement for :wx_misc.getOsDescription/0, implementing it across the Browser, Json, and Wx backends along with corresponding tests. Feedback suggests simplifying the nested case statements in os_description/0 for better readability, and removing a redundant Process.delete/1 call inside the test's on_exit/1 callback since it runs in a separate process.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread lib/desktop/platform/system.ex Outdated
Comment on lines +52 to +61
case raw do
nil ->
nil

str ->
case String.trim(str) do
"" -> nil
trimmed -> trimmed
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The nested case statements can be simplified to a single if block, which improves readability and is more idiomatic Elixir.

    if raw do
      case String.trim(raw) do
        "" -> nil
        trimmed -> trimmed
      end
    end

Comment on lines +14 to +17
previous = Application.get_env(:desktop, :backend, :auto)
Application.put_env(:desktop, :backend, StubBackend)

on_exit(fn ->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In ExUnit, on_exit/1 callbacks run in a separate process, not the test process. Therefore, calling Process.delete(:stub_os_description) inside on_exit is a no-op as it deletes the key from the cleanup process's dictionary rather than the test process's dictionary. Since the test process terminates and its process dictionary is automatically garbage collected, you can safely remove this line.

    on_exit(fn ->
      Application.put_env(:desktop, :backend, previous)
    end)

@dominicletz dominicletz added the cursor-waiting-for-ci Cursor issue-workflow (cursor-automation.com) label Jul 21, 2026
@cursor-automation cursor-automation Bot added cursor-demo Cursor issue-workflow (cursor-automation.com) cursor-waiting-for-human Cursor issue-workflow (cursor-automation.com) and removed cursor-waiting-for-ci Cursor issue-workflow (cursor-automation.com) cursor-demo Cursor issue-workflow (cursor-automation.com) labels Jul 21, 2026
@dominicletz

Copy link
Copy Markdown
Collaborator Author

@cursoragent rebase this on master and fix the gemini comments

@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

Rebased onto main and addressed both Gemini notes on #75:

  • os_description/0 now uses a single if raw do … end for blank trimming
  • Removed the no-op Process.delete/1 from on_exit

Related tests, guards, and wx tests passed.

Open in Web Open in Cursor 

Use a single if/case for blank normalization per review feedback, and
remove Process.delete from on_exit (runs in a different process).

Co-authored-by: Dominic Letz <dominicletz@users.noreply.github.com>
@cursor
cursor Bot force-pushed the cursor/pull-request-74-review-9f20 branch from dac0db9 to a71b3b5 Compare July 21, 2026 15:44
@cursor cursor Bot changed the title Review fixes for #74: os_description style + blank normalization tests Simplify os_description trim; rebase on main Jul 21, 2026
@dominicletz
dominicletz marked this pull request as ready for review July 21, 2026 15:45
@dominicletz
dominicletz merged commit 9176365 into main Jul 21, 2026
2 checks passed
@dominicletz
dominicletz deleted the cursor/pull-request-74-review-9f20 branch July 21, 2026 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cursor-waiting-for-human Cursor issue-workflow (cursor-automation.com)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants