Update docs and include a powershell setup script #94
Conversation
WalkthroughREADME now requires cloning submodules and offers multiple Windows setup paths (Setup.bat, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
scripts/setup.ps1 (3)
5-5: Add existence check for premake5.exe.The script attempts to run
vendor\bin\premake5.exewithout verifying it exists. If the file is missing (e.g., submodules weren't cloned), the error message won't clearly indicate the root cause.🔎 Proposed fix
+if (-not (Test-Path "vendor\bin\premake5.exe")) { + Write-Host "Error: premake5.exe not found. Did you clone with --recurse-submodules?" -ForegroundColor Red + Read-Host "Press Enter to continue..." + exit 1 +} + & "vendor\bin\premake5.exe" vs2022
1-12: Consider returning explicit exit codes for automation.The script doesn't explicitly return exit codes, which makes it difficult for automated build systems or CI/CD pipelines to determine if the setup succeeded. The
$ErrorActionPreference = "Stop"helps, but the catch block swallows the error without re-throwing or setting an exit code.🔎 Proposed fix
catch { Write-Host "Error: $($_.Exception.Message)" -ForegroundColor Red + Pop-Location + Read-Host "Press Enter to continue..." + exit 1 } finally { Pop-Location } - -Read-Host "Press Enter to continue..." + +Read-Host "Press Enter to continue..." +exit 0
14-14: Read-Host blocks automation and non-interactive usage.The unconditional
Read-Hostprompt prevents the script from being used in automated or CI/CD environments. Consider making this prompt optional or removing it for non-interactive scenarios.🔎 Proposed fix
-Read-Host "Press Enter to continue..." +if ([Environment]::UserInteractive) { + Read-Host "Press Enter to continue..." +}README.md (1)
39-41: Use correct code block identifier.The code block uses
bashidentifier for a command that works across platforms including Windows cmd.exe. Consider usingshellorconsolefor platform-agnostic commands, or specify the actual shell if targeting a specific environment.🔎 Proposed fix
-``` bash +``` shell premake5 vs2022</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Repository UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3b8e414fdecfc6c8b58816106fe8d912bd172e31 and 25da8d7f7c530392b86b245dd68dda96ba074914. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `README.md` (1 hunks) * `scripts/setup.ps1` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [style] ~16-~16: Consider a more concise word here. Context: ... a recent version) ## Getting Started In order to clone the repository, you'll need to gr... (IN_ORDER_TO_PREMIUM) --- [style] ~17-~17: Consider a more expressive alternative. Context: ... you'll need to grab the submodules. To do that, you will need the following comma... (DO_ACHIEVE) --- [grammar] ~45-~45: Ensure spelling is correct Context: ...e setup and ready to go. ### 3rd party libaries - [Dear ImGui](https://github.com/ocornut/i... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) --- [typographical] ~47-~47: This greeting should probably end with a comma. Context: ...eady to go. ### 3rd party libaries - [Dear ImGui](https://github.com/ocornut/imgui) - [G... (EN_GREETING_WITHOUT_COMMA) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>README.md</summary> 25-25: Emphasis style Expected: underscore; Actual: asterisk (MD049, emphasis-style) --- 25-25: Emphasis style Expected: underscore; Actual: asterisk (MD049, emphasis-style) --- 31-31: Emphasis style Expected: underscore; Actual: asterisk (MD049, emphasis-style) --- 31-31: Emphasis style Expected: underscore; Actual: asterisk (MD049, emphasis-style) --- 37-37: Emphasis style Expected: underscore; Actual: asterisk (MD049, emphasis-style) --- 37-37: Emphasis style Expected: underscore; Actual: asterisk (MD049, emphasis-style) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
README.md
Outdated
| *on Windows command line:* | ||
|
|
||
| ``` bash | ||
| ./scripts/setup.ps1 | ||
| ``` | ||
|
|
||
| *on Windows Powershell:* | ||
|
|
||
| ``` bash | ||
| ./scripts/Setup.ps1 | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify the distinction between setup options.
The current wording creates confusion:
- "Windows command line" (lines 25-29) vs "Windows Powershell" (lines 31-35) is unclear since PowerShell is a command line interface
- Both instructions run the same
.ps1script, which is a PowerShell script - The distinction likely intended is: running from cmd.exe vs running from PowerShell
Additionally, the code blocks use bash identifier for PowerShell commands, which is incorrect.
🔎 Proposed fix
-*on Windows command line:*
+*from cmd.exe (Command Prompt):*
-``` bash
+``` cmd
./scripts/setup.ps1-on Windows Powershell:
+from PowerShell:
- bash -./scripts/Setup.ps1 + powershell
+./scripts/setup.ps1
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
25-25: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
25-25: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
31-31: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
31-31: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🤖 Prompt for AI Agents
In README.md around lines 25 to 35 the instructions confuse "Windows command
line" with PowerShell, use incorrect code block language tags, and show
inconsistent script casing; change the headings to "from cmd.exe" and "from
PowerShell", ensure both blocks call the same ./scripts/setup.ps1 (lowercase)
consistently, and update the fenced-code block language identifiers to `cmd` for
the cmd.exe example and `powershell` for the PowerShell example so the commands
and labels are accurate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
25-41: Resolve setup instruction issues from previous review (still unaddressed).The following issues from the previous review remain unresolved and impact usability and correctness:
- Line 25: Wording "command standard line" is confusing; should be "from cmd.exe"
- Line 27 & 33: Code blocks incorrectly use
bashidentifier for Windows shell commands (cmdfor cmd.exe,powershellfor PowerShell)- Line 34: Filename case inconsistency (
Setup.ps1should besetup.ps1to match the actual file and ensure consistency across lines 28 and 34)- Lines 25, 31, 37: Emphasis style violations—asterisks should be replaced with underscores per Markdown linting rules (MD049)
🔎 Comprehensive fix for all issues
-*on Windows command standard line (cmd.exe):* +_from cmd.exe (Command Prompt):_ -``` bash +``` cmd ./scripts/setup.bat-on Windows Powershell:
+from PowerShell:-
bash -./scripts/Setup.ps1 +powershell
+./scripts/setup.ps1-*or, if you have premake already installed:* +_or, if you have premake already installed:_ ``` bash premake5 vs2022
🧹 Nitpick comments (1)
README.md (1)
17-21: Optional: Refine wording for clarity and conciseness.Line 17 uses conversational phrasing ("In order to," "grab") that could be more direct and professional. This is a minor style improvement.
🔎 Suggested refinement
-In order to clone the repository, you'll need to grab the submodules. To do that, you will need the following command line: +To clone the repository with submodules, run:
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~16-~16: Consider a more concise word here.
Context: ... a recent version) ## Getting Started In order to clone the repository, you'll need to gr...
(IN_ORDER_TO_PREMIUM)
[style] ~17-~17: Consider a more expressive alternative.
Context: ... you'll need to grab the submodules. To do that, you will need the following comma...
(DO_ACHIEVE)
[typographical] ~47-~47: This greeting should probably end with a comma.
Context: ...ady to go. ### 3rd party libraries - Dear ImGui - [G...
(EN_GREETING_WITHOUT_COMMA)
🪛 markdownlint-cli2 (0.18.1)
README.md
25-25: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
25-25: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
31-31: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
31-31: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
37-37: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
37-37: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
31-35: Fix case inconsistency in PowerShell script filename.Line 34 references
./scripts/Setup.ps1(with uppercase 'S'), but the new PowerShell script file added in this PR isscripts/setup.ps1(lowercase). This inconsistency will confuse users and may cause issues on case-sensitive systems.🔎 Proposed fix
_on Windows Powershell:_ ``` powershell -./scripts/Setup.ps1 +./scripts/setup.ps1</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Repository UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 615610c50509802e42c2114021b0fa5f9a801294 and a01a6aea88c9f2dec91e8b7eb46325ab8b190cb2. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [grammar] ~17-~17: Ensure spelling is correct Context: ...rsion) ## Getting Started In order to fuly clone the repository run the following ... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) --- [typographical] ~47-~47: This greeting should probably end with a comma. Context: ...ady to go. ### 3rd party libraries - [Dear ImGui](https://github.com/ocornut/imgui) - [G... (EN_GREETING_WITHOUT_COMMA) </details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>README.md (3)</summary><blockquote> `25-29`: **LGTM.** The distinction between cmd.exe and PowerShell is now clear, and the code block identifier is correct. --- `37-41`: **LGTM.** The premake option is clearly presented as an alternative setup method. --- `43-54`: **LGTM.** The Getting Started flow is now clear with submodule cloning prerequisites, and the 3rd party libraries section is well-documented with proper links. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
17-17: Optional: Consider more concise phrasing.The phrase "In order to fully clone" could be shortened to "To fully clone" or "To clone" for better readability, though this is a minor style preference.
🔎 Suggested revision
-In order to fully clone the repository run the following command: +To fully clone the repository run the following command:
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~16-~16: Consider a more concise word here.
Context: ... a recent version) ## Getting Started In order to fully clone the repository run the foll...
(IN_ORDER_TO_PREMIUM)
[typographical] ~47-~47: This greeting should probably end with a comma.
Context: ...ady to go. ### 3rd party libraries - Dear ImGui - [G...
(EN_GREETING_WITHOUT_COMMA)
🔇 Additional comments (2)
README.md (2)
25-41: Great! Setup instructions are now clear and correct.The distinction between "on Windows using the command prompt cmd.exe:" (lines 25-29) and "on Windows Powershell:" (lines 31-35) is now properly clarified. The code block language identifiers (
cmdandpowershell) are correctly applied, and the script filename is consistently referenced as lowercase./scripts/setup.ps1. The addition of the premake alternative (lines 37-41) provides flexibility for users who have it installed.
19-21: Submodule cloning requirement is well-documented.The
git clone --recurse-submodulescommand correctly ensures users clone the repository with all required submodules on first setup, which is a good improvement for first-time setup clarity.
1 participant
The following changes have been implemented: