Conversation
Bombe
left a comment
Bombe
left a comment
There was a problem hiding this comment.
I’m not familiar with what this is supposed to be doing but I’m a little bit appalled by all the “instanceof String / Long / …” if-cascades… are those really necessary?
| Date modified = (Date)map.get("modified"); | ||
| if (totalPagesObj instanceof String) { // FIXME | ||
| totalPages = Long.parseLong((String) totalPagesObj); | ||
| } else if (totalPagesObj instanceof Long) { // FIXME yaml??? It seems to give a Long if the number |
There was a problem hiding this comment.
You could use Number and it’s longValue() method.
| ProtoIndexComponentSerialiser cmpsrl = ProtoIndexComponentSerialiser.get((Integer)map.get("serialFormatUID"), subsrl); | ||
| Object serialFormatUIDObj = map.get("serialFormatUID"); | ||
| int serialFormatUID; | ||
| if (serialFormatUIDObj instanceof String) { // FIXME |
There was a problem hiding this comment.
Does everything in this map potentially exist as String? Where does this map come from?
There was a problem hiding this comment.
I could write this in the situation when I met a String there during debugging. Dirty. In my opinion it starts with "snakeyaml implicit resolver", which I turned off as a result.
| if (s == null) { | ||
| throw new IllegalArgumentException("can't have a null subject!"); | ||
| // throw new IllegalArgumentException("can't have a null subject!"); | ||
| s = "null"; // FIXME |
There was a problem hiding this comment.
Hmm, this FIXME in combination with the removed exception does not inspire confidence. How is using "null" as subject better than getting an exception and fixing the cause of that?
There was a problem hiding this comment.
Probably related to https://github.com/freenet/plugin-Library/pull/16/files#diff-aa254c0343e62a1fcd16f7077a106f09R302. Not sure if that gives away a bit more of context.
There was a problem hiding this comment.
As far as I can remember this situation, this is about the fact that the subject can contain the human word "null", but then it turns into null (smart library mode)
There was a problem hiding this comment.
That sounds like something that should be fixed in a different place… but as with the rest of this PR I’m totally not qualified to talk about any of it because I have no clue of all the contexts in question. :)
| assertTrue(m.get("key2") instanceof Bean); // NOTE these tests fail in snakeYAML 1.2 and below, fixed in 1.3 | ||
| assertTrue(m.get("key3") instanceof Custom); | ||
| assertTrue(m.get("key4") instanceof Wrapper); | ||
| assertTrue(map.get("key1") instanceof Bean); |
There was a problem hiding this comment.
This only tests for the general existence in the map but does not validate the values… at least for key4 I would expect a bit more here. 😃
4 participants
This PR contains the work done to upgrade SnakeYAML to 1.25 (latest version), to compile against java 8, and changes related to fred.
It's a partial work of #15