-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add interfaces in c# bindings to make testing easier #1837
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
base: master
Are you sure you want to change the base?
Conversation
gingters
left a comment
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.
Regarding the added exceptions, in general:
First and foremost, there aren't any other implementations that could be passed in. So the actual type checks aren't that helpful.
Secondly, this (currently) is a very thin wrapper around the underlying C++ api.
The introduced type checks now start adding verification logic to the wrapper.
While a reasonable guarding oftentimes helps with development, I don't really see this adding value here in this case and it changes a simple 'passthrough' call to the actual API into real methods that now add logic and don't provide any tests testing that logic.
For now, I would refrain from adding logic, even if its only a simple typecheck / exception, to the wrapper, until we decide how the API design of the C# wrapper should look like going forward.
| @@ -0,0 +1,13 @@ | |||
| namespace RPiRgbLEDMatrix | |||
| { | |||
| public interface IRGBLedMatrix | |||
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.
Why not IDisposable on the interface here, as the actual type is disposable?
| public int DrawText(RGBLedFont font, int x, int y, Color color, string text, int spacing = 0, bool vertical = false) | ||
| { | ||
| if (font is not RGBLedFont rgbLedFont) |
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.
That check doesn't really make sense, as the parameter wasn't changed to the interface?
| GpioSlowdown= 4 | ||
| }; | ||
|
|
||
| using var leds = new RGBLedMatrix(options); |
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.
Everywhere else it's matrix, not leds. When unifying the initialization in the samples, it would make sense to refactor that variable naming here too ;)
| if (string.IsNullOrEmpty(bdfFontPath)) | ||
| throw new ArgumentException("Font path cannot be null or empty.", nameof(bdfFontPath)); | ||
|
|
||
| if (File.Exists(bdfFontPath) == false) |
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.
Is there any reason this cannot be !File.Exists(bdfFontPath)?
| Cols= 64, | ||
| Rows= 64, | ||
| GpioSlowdown= 4 | ||
| }; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
As of now, I wouldn't really care about formatting. At a future point in time, I would throw a default .editorconfig with csharpier rules in the project, have that formatted automatically in a huge refactor: global reformatting commit, and from there on we simply rely on automatic formatting with running dotnet csharpier every time before committing and live happily forever after 😅
|
|
||
| canvas.DrawText(font, 1, 6, new Color(0, 255, 0), text); | ||
| canvas.DrawText(font, 1, 16, new Color(0, 255, 0), text); | ||
| matrix.SwapOnVsync(canvas); |
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.
How about adding the comment like following?
// You may need to adjust the drawing position depending on the font size.
|
It is difficult to agree to adding interfaces only for testing without being provided with actual test code. The wrapper class API is not yet stable and may change in future work. The following is a proposal for the test implementation. + public interface IRGBLedCanvasFactory {
+ IRGBLedCanvas Create(...);
+ }
public class RGBLedMatrix {
+ RGBLedMatrix(IRGBLedCanvasFactory canvasFactory)
+ {
+ rgbLedCanvasFactory = canvasFactory;
+ }
- public IRGBLedCanvas CreateOffscreenCanvas() => new RGBLedCanvas(led_matrix_create_offscreen_canvas(matrix));
+ public IRGBLedCanvas CreateOffscreenCanvas() => rgbLedCanvasFactory.Create(...);
}Before adding the interfaces, I think we should discuss and clarify what and how we're testing it. |
|
@UniqueDaniel can you please address the changes requested as applicable and confirm the end result is fully tested. |
|
Is this PR still being worked on? If not, I'll close it. It's not that I don't want to to see this get in, but we don't want stale/abandoned PRs to stay around forever either. |
|
It will be soon. I have been in hospital and away from home so not the best places to pick it up. I will drop the variable checks from the PR as they appear to be controversial for now, and neaten up the examples a little further. Not for this PR, but are there any preferred c# code styles people would like to work towards? |
|
In my opinion, it would be good to maintain consistency within the existing code. For now, we should focus on the actual working code, and style adjustments can come later. |
Main objective here is to add interfaces to the bindings to allow a user to test their code without needing to go to the matrix. I have been doing this in my projects for some time as I dev in windows then publish to rpi, writing tests against a fake interface makes life a lot easier.
While testing I also ununified the examples to make configuring for unique test rigs much easier.