Merge json #23
Merge json #23
Conversation
|
|
||
| public static class Controller { | ||
| public int port = -1; | ||
| public String type = null; |
There was a problem hiding this comment.
Could we use an enum for types instead of strings? I feel like this would be less prone to typos and runtime errors that way.
There was a problem hiding this comment.
I will look into it
There was a problem hiding this comment.
This will probably be a new pull request because I want to add some features to JSONSync to make this easier first
|
|
||
| public String toString() { | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
| stringBuilder.append("Controler Object {\n"); |
There was a problem hiding this comment.
Tiny nitpick but I think there are 2 L's in controller.
There was a problem hiding this comment.
Should we keep the toString method, because I made it for testing?
There was a problem hiding this comment.
If its needed for unit tests then keep it, otherwise we could probably get rid of it.
There was a problem hiding this comment.
Still need to figure out how to do unit tests again, but I will remove it
| } catch (NumberFormatException e) { | ||
| if (!Controllers.synced.getObject().buttonShorthands.containsKey(button)) | ||
| throw new RuntimeException( | ||
| "Button Id not found as interger or in shorthands " + button); |
There was a problem hiding this comment.
Another tiny nitpick but I think you accidentally put an extra 'r' before the 'g' in 'integer'.
There was a problem hiding this comment.
I will fix that, and I also need to capitalize the d in Id
| List<Controllers.Controller> controllers = Controllers.getControllers(); | ||
| System.out.println(controllers.size()); | ||
| assertEquals(23124, controllers.size()); | ||
| assertFalse(true); |
There was a problem hiding this comment.
Forgot to redo them I will that later today
There was a problem hiding this comment.
I was trying to get unit testing to work
There was a problem hiding this comment.
Oh yeah I remember they weren't running before right?
There was a problem hiding this comment.
Did you get unit tests working for the monitors?
There was a problem hiding this comment.
Yeah I got them to run as far as I remember
|
oh you might have to merge |
|
@avidraccoon what's the status of this PR? I think we should make a branch on high-key-2024 to test this and then merge to main when we get it to work |
|
yes and I am realizing how bad this pull requests name is |
Modifys controllers to use new JSON system