Add dynamic callbacks mechanism as alternative for StructSink #89
Conversation
Current coverage is 99.14% (diff: 100%)@@ master #89 diff @@
==========================================
Files 78 81 +3
Lines 1003 1049 +46
Methods 0 0
Messages 0 0
Branches 181 186 +5
==========================================
+ Hits 994 1040 +46
Misses 1 1
Partials 8 8
|
|
Coverage went down, I think we can remove some dead code from the |
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "stream: " + input + "; offset: " + offset + "; order: " + order; | ||
| return "stream: " + input + "; offset: " + offset + "; order: " + order + "; callbacks: " + callbacks; |
There was a problem hiding this comment.
(Apparently there is a new way of reviewing on GitHub, let's see!)
We need a test for toString() with actual callbacks. The current toString() tests Environments without callbacks.
| handleCallbacks(environment.callbacks, token); | ||
| } | ||
|
|
||
| private void handleCallbacks(final TokenCallbackList callbacks, final Token token) { |
There was a problem hiding this comment.
I don't think we have explicit tests yet for this code, add some.
There was a problem hiding this comment.
Excellent point! I had a hard time coming up with a good test however, which indicated that it probably shouldn't be a public API. To fix this I moved the method to the Token base class, which greatly simplified the implementation.
| import io.parsingdata.metal.data.ParseResult; | ||
| import io.parsingdata.metal.token.Token; | ||
|
|
||
| public class BaseCallback implements Callback { |
There was a problem hiding this comment.
Add some tests for this class.
There was a problem hiding this comment.
It is tested, but because it is not abstract, coverage requires a test that doesn't inherit from BaseCallback in order to execute the handleSuccess() and handleFailure() methods. I've reverted it to an abstract class.
Clients can trivially recreate the adapter pattern if they need it. Providing an abstract class seems cleaner for Metal I think.
|
|
||
| public interface Callback { | ||
|
|
||
| void handle(final Token token, final ParseResult result); |
There was a problem hiding this comment.
Let's get into the habit of adding Javadoc to new code...
There was a problem hiding this comment.
I've added #9 to the 5.0.0 milestone. It's better to document the important stuff first I think.
| return new TokenCallbackList(checkNotNull(head, "head"), this); | ||
| } | ||
|
|
||
| public TokenCallbackList add(final TokenCallbackList list) { |
There was a problem hiding this comment.
I think this method is dead code, remove it.
| @@ -105,7 +105,7 @@ public void encoding() { | |||
| @Test | |||
| public void data() { | |||
| final Environment environment = stream(1, 2); | |||
| final String envString = "stream: InMemoryByteStream(2); offset: 0; order: graph(EMPTY)"; | |||
| final String envString = "stream: InMemoryByteStream(2); offset: 0; order: graph(EMPTY); callbacks: "; | |||
There was a problem hiding this comment.
(See my earlier "we need a test for toString()" 😉)
| @@ -25,7 +26,7 @@ | |||
|
|
|||
| @Test | |||
| public void getValueOnEmpty() { | |||
| assertNull(ByName.getValue(ParseGraph.EMPTY, "name")); | |||
| TestCase.assertNull(ByName.getValue(ParseGraph.EMPTY, "name")); | |||
There was a problem hiding this comment.
Not sure what happened here, but use a static import as before. Also: I think we shouldn't use TestCase.* but Assert.*.
…Fixed Def's toString(). Removed obsolete code from TokenCallbackList.
…ed an unused one. Reverted BaseCallback to abstract class.
…from the public API.
Resolves #12.