-
Notifications
You must be signed in to change notification settings - Fork 41
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
Challenge 4.1 should be clarified, solution/test possibly changed #10
Comments
@KevinGreene I clarified this to simply use batch, and log before processing. Thanks - we'll add a better explanation of lifecycles when we close #5. We really appreciate the feedback! |
Yeah, there's still something unclear about this one. I was moving along smoothly until I got to 4-1. There were two things that I think still need fixing:
|
@neverfox I'll take another shot at this one this afternoon, thanks for the report! |
@MichaelDrogalis Thanks! This is a fantastic tutorial. If only every library came with something like this. |
@neverfox Thank you! :) I really appreciate that. |
This is partially referenced in #5, but I think during the workshop it was misdiagnosed.
In the test file, it says:
I believe this should say :onyx.core/results, as it is asking for the results after it has been processed, which should indicate that :onyx.core/batch shouldn't be used.
However, this propagated to the test / solutions files as well, as the test rebuilds the input instead of using the expected output here - https://github.com/onyx-platform/learn-onyx/blob/master/test/workshop/jobs/test_4_1.clj#L47
The solution also uses :onyx.core/batch to make the tests pass as well, seen here - https://github.com/onyx-platform/learn-onyx/blob/answers/src/workshop/challenge_4_1.clj#L53
I'd be happy to change this and submit a PR, but should the change be reworking the test, solution, and hints to use :onyx.core/results, or should it be reworking the prompt to simply say "every segment that we see before it's processed..."?
The text was updated successfully, but these errors were encountered: