-
Notifications
You must be signed in to change notification settings - Fork 11
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
Debug V3 Re-Write #158
base: 2.6.2
Are you sure you want to change the base?
Debug V3 Re-Write #158
Conversation
- Removed Enable Debug Setting - Added DebugToConsole and DebugToFile Settings - Debug settings now take lists of Debug Level Names - Added DebugLevels#match - Old Debug setting reading moved to DebugV2 - New debug settings run with DebugV3 - Common code is contained in Debug - Removed old/un-usable config fixes
- Added isNull method to check before use - Added check for NULL value and reset/re-read setting if true - Catch-up to 2.6.2 - Added JSON_SAVING and JSON_LOADING debug
- Changed isMap call to isList(whoops...) - Changed return type to Optional<List<String>> - Changed boolean check in Debug to utilize Optional#orElse
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.
Looks good but I dont agree with the Debug class hierarchy... It should not be this complex
Also look at the generics thing and make sure that works as expected
|
||
return !debugDestination.isEmpty(); | ||
} | ||
} |
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 is this thing still here?
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.
returns enable/disable for the system. If nothing is loaded debug should be disabled.
|
||
return !debugToConsole.isEmpty(); | ||
} | ||
} |
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.
just get rid of DebugV2 and merge this with Debug.
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.
Made seperate so that I can break out later. Still not sure if this will make it to full release
@@ -115,6 +121,21 @@ public Map<String, Object> asMap() { | |||
return null; | |||
} | |||
|
|||
public boolean isList() { | |||
return !isNull() && obj instanceof List; |
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.
not sure this works... you need to be careful with generics
class ObjectHolder<T>
means you need to check obj instanceof List<? extends T>
this is not the same, List<String>
is not compatible or a subclass of List<Object>
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.
you right, thats a goof.
Well, it shouldn't be ? extends T because the T would be a list(and most likely a String List)
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.
Yes you're right, I meant T is whatever obj is a list of... maybe instanceof List<?> should be fine then?
This still needs to have File Logging Implemented but other than that It should be good to go...
Pull Request Etiquette
There are several guidelines you should follow in order for your Pull Request to be merged.
Replace
[ ]
with[x]
to cross the checkbox.Changes
Replace this sentence with a description of che changes introduced by this PR.