Json support#134
Conversation
|
|
| public class CustomItemJson implements JsonDeserializer<Item> { | ||
| @Override | ||
| public Item deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { | ||
| return Item.getByNameOrId(json.getAsString()); |
There was a problem hiding this comment.
Shouldn't this be pulling from ForgeRegistries.ITEM?
| @Override | ||
| public NBTTagCompound deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { | ||
| try { | ||
| return JsonToNBT.getTagFromJson(json.toString()); |
There was a problem hiding this comment.
Does this allow for doing straight JSON objects as json? or just as escaped strings.
| try { | ||
| return JsonToNBT.getTagFromJson(json.toString()); | ||
| } catch (NBTException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Shouldn't you be rethrowing as a JsonParseException? Rather than just eating the error and puking into the log?
| } | ||
|
|
||
| /* | ||
| Not needed for now |
There was a problem hiding this comment.
Then Remove it. Don't leave commented out code to just sit.
| public ItemInfo deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { | ||
| JsonObject obj = json.getAsJsonObject(); | ||
|
|
||
| Item item = context.deserialize(obj.get("item"), Item.class); |
There was a problem hiding this comment.
What if Item is null? Should probably have some sorta of better validation here.
| registerDeserializer(NBTTagCompound.class, new CustomNBTJson()); | ||
| } | ||
|
|
||
| public static void registerDeserializer(Type type, JsonDeserializer deserializer) { |
There was a problem hiding this comment.
Shouldn't this be accessible through the api somehow?
There was a problem hiding this comment.
I am leaving this up to pup, as I am not sure how he wants the API to look like.
There was a problem hiding this comment.
I think this is probably fine given the JSON stuff will probably be for a larger version such as 2.0 which will allow for refactoring a bunch of stuff without worrying about breaking API things. Given a bunch of the stuff in LevelLockHandler should probably be moved from the base package to an API package. The only reason it is in LevelLockHandler to begin with is that it basically grew as I was fixing and then expanding on things making them more generic and supporting custom registrations.
|
|
||
| public static void setupLocks() { | ||
| registerDefaultLockKeys(); | ||
| if (configLocks != null) { |
There was a problem hiding this comment.
Wasn't this code for pulling from the config file?
There was a problem hiding this comment.
I looked into how we are doing it currently and the setupLocks method is called in FMLPostInitializationEvent so we use it to register the lock keys, and then if the config has locks we register them. We previously loaded the strings here https://github.com/Coders-After-Dark/Reskillable/blob/develop/1.12.2/src/main/java/codersafterdark/reskillable/base/ConfigHandler.java#L53 Looking at the new code the locks are loaded from json here (which makes sense as all custom locks should have been registered by now from addons). I think this is fine however I think the linked line and LevelLockHandler#loadFromConfig and private static String[] configLocks; should probably be removed as they are no longer used. That or an automated converter should be made to port existing config files to the JSON format. I will mark through the review the lines that should be able to be deleted.
pupnewfster
left a comment
pupnewfster
left a comment
There was a problem hiding this comment.
ConfigHandler description of locks through the line LevelLockHandler.loadFromConfig(locks) can also be removed or have a converter be created.
| @@ -42,44 +43,27 @@ public class LevelLockHandler { | |||
| private static Map<Class<?>, List<Class<? extends LockKey>>> lockTypesMap = new HashMap<>(); | |||
| private static Map<LockKey, Set<FuzzyLockKey>> fuzzyLockInfo = new HashMap<>(); | |||
| private static String[] configLocks; | |||
There was a problem hiding this comment.
Can be removed, same with the DEFAULT_SKILL_LOCKS a few lines up but it is not letting me mark it.
| registerLockKeyName("nbt", GenericNBTLockKey.class); | ||
| } | ||
|
|
||
| public static void loadFromConfig(String[] configValues) { |
There was a problem hiding this comment.
Why should this be able to be removed?
This is needed for the json parser to deserialize the key name.
There was a problem hiding this comment.
The loadFromConfig(String[] configValues) method. Github in its infinite wisdom decided to also display those lines of your code.
| mainConfig.load(); | ||
| loadData(); | ||
| loadJSONLocks(); | ||
| // loadJSONLocks(); |
There was a problem hiding this comment.
Can remove this line as it is being loaded from LevelLockHandler#setupLocks and there is no reason to have this here just commented out.
4 participants
No description provided.