Conversation
❌ Deploy Preview for splashkit failed.
|
ralphweng-autograb
left a comment
There was a problem hiding this comment.
Peer Review
I've reviewed the replace_all usage example. The example is simple and demonstrates the function clearly — replacing all occurrences of "foo" with "bar" is easy to understand.
However, there are several issues to address:
Issues
Usage Example Files
-
C++ uses
using namespace std;. This is not needed and not standard for SplashKit examples. SplashKit'sstringtype is available through#include "splashkit.h"— removeusing namespace std;. -
Top-level C# uses OOP-style calls. The top-level C# file should use
using static SplashKitSDK.SplashKit;and call functions directly (e.g.,ReplaceAll(...),WriteLine(...)) instead ofSplashKit.ReplaceAll(...). The current code is nearly identical to the OOP version. -
OOP C# missing namespace. The OOP C# file should wrap the class in a namespace:
namespace ReplaceAllExample { ... }. -
Missing newline at end of files. All code files are missing a trailing newline.
usage-example-references.json Issues
-
The
replace_allreference entry lists"print"as the only function. This should list the actual SplashKit functions used:replace_all,write_line. -
Unrelated entries added. This PR adds cross-reference entries for
saturation_of,rectangle_around,bitmap_center,draw_circle,get_font_style, andclose_window— none of which are related to thereplace_allexample. These should be submitted in their own PRs or with the corresponding usage example PRs. Please remove them from this PR to keep it focused.
guides-groups.json Changes
- Unrelated change to
guides-groups.json. This PR capitalises"camera"→"Camera"and"graphics"→"Graphics"in the guides groups config and reorders entries. This is unrelated to thereplace_allexample and could break guide generation. Please revert this change.
Checks
- All required files are present.
- Example Title (.txt)
- C++ code (SplashKit)
- C# code (top-level statements)
- C# code (Object-Oriented Programming)
- Python code
- Output screenshot (.png)
Code Tests done
- C++ SplashKit code ran correctly.
- C# top level code ran correctly.
- C# OOP code ran correctly.
- Python code ran correctly.
Note: I was unable to compile and run the code locally. Please verify all 4 variants run correctly and produce the expected output.
ralphweng2023
left a comment
There was a problem hiding this comment.
Left some inline comments on the specific files — see above.
| @@ -0,0 +1,16 @@ | |||
| #include "splashkit.h" | |||
| using namespace std; | |||
There was a problem hiding this comment.
You don't need using namespace std; here — SplashKit handles string through #include "splashkit.h". Just remove this line.
| @@ -0,0 +1,10 @@ | |||
| using SplashKitSDK; | |||
There was a problem hiding this comment.
For top-level C#, use using static SplashKitSDK.SplashKit; so you can call functions directly like ReplaceAll(...) and WriteLine(...) without the SplashKit. prefix. Right now this reads the same as the OOP version.
| @@ -0,0 +1,16 @@ | |||
| using SplashKitSDK; | |||
|
|
|||
| public class Program | |||
There was a problem hiding this comment.
Wrap this in a namespace per convention, e.g.:
namespace ReplaceAllExample
{
public class Program
{
...
}
}| @@ -1,10 +1,10 @@ | |||
| [ | |||
| "Camera", | |||
There was a problem hiding this comment.
This capitalisation change (camera → Camera, graphics → Graphics) looks unrelated to the replace_all example. Could potentially break guide generation — best to revert this file and submit separately if needed.
| ] | ||
| } | ||
| ], | ||
| "color": [ |
There was a problem hiding this comment.
This saturation_of entry isn't related to the replace_all example — should go in the PR that adds the saturation_of usage example instead. Same goes for the rectangle_around, bitmap_center, draw_circle, get_font_style, and close_window entries below. Keep this PR focused on just replace_all.
| }, | ||
| { | ||
| "funcKey": "replace_all", | ||
| "title": "Replacing all occurrences of a word in a sentence", |
There was a problem hiding this comment.
The functions list here just has "print" — should be "replace_all" and "write_line" (the actual SplashKit functions used in the example).
Description
Added a usage example for the replace_all function in the utilities category.
The example demonstrates replacing all occurrences of "foo" with "bar" in a sentence across C++, C#, and Python.
Type of change
How Has This Been Tested?
The example output was verified manually to ensure all occurrences of the target substring are correctly replaced.
Testing Checklist