-
Notifications
You must be signed in to change notification settings - Fork 3
Fix multiline expectations #35
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
Conversation
7004d19 to
90779d6
Compare
|
Thanks @mattiuusitalo! I am taking a peek at your change on my dev box. If I understand it correctly, we can still distinguish stdout from eval when stdout contains the For example: But, we don't force the single semicolon line continuation on the user if there is no ambiguity from the stdout output: If this is what you intended, I like it. Same idea for ambiguous evals, right? In the oddball case where needed to, we would not do this: But instead this: Have I understood your thinking? |
75f0bac to
5504d80
Compare
|
First of all, rebased the branch with the latest changes from main, so heads up if you try to pull from the same branch.
I didn't anticipate the case where =stdout=> or =clj=> would be part of the expected output. I'm happy that the single semicolon case makes it work though. That would retain the backwards compatibility with earlier versions, right? I noticed that this is a very specific case with just the symbols =stdout=> =stderr=> =clj=> etc being printed. For users of this library that wouldn't be a problem since they could just use different symbols, I think. Making a mini DSL like this that still retains the human readable aspect of the code blocks, which is after all their original intention, is surprisingly challenging I must say. I also added a couple of tests based on your examples. |
Thanks for the heads up. In the future, a regular merge from main is fine.
Yeah! Thanks for sticking it out! I'm looking forward to reviewing your changes soon! |
lread
left a comment
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.
I think this looks good!
Supporting multiline commented eval values adds complexity.
Because of this, I think I'd add more tests like the larger-test.
If you've had enough, I could take a shot at it.
I'd also expand on the docs a bit and explain more options for multiline. I can also take a stab at this.
|
@mattiuusitalo, just an FYI, I'll go ahead and collaborate by making commits to this PR. |
Create a dedicated section to explain multiline output for example.md and example.adoc. These both explain to the user and act as tests for test-doc-blocks.
|
Test-doc-blocks has the luxury of having only a handful of fans/users. I've tried this PR against known users of test-doc-blocks:
I found breakages for etaoin and honeysql. They are all of the same flavour. This is good, we only found one problem, and it is easy to understand. An example code block from etaoin: [source,clojure]
----
;; switch to the first top-level iframe with the main page: frame1
(e/switch-frame-first driver)
;; downward to the first iframe with frame1: frame2
(e/switch-frame-first driver)
(e/get-element-text driver :in-frame2)
;; => "In frame2 paragraph"
;; back up to frame1
(e/switch-frame-parent driver)
;; back up to main page
(e/switch-frame-parent driver)
----The Because an evaluation results in a form, and we already depend on rewrite-clj which parses forms, we can probably more intelligently figure out what our test expectation should be here and not include any unwanted comments. I'll explore this idea. |
…tations * upstream/main: dev: add bb `dev` task to launch a REPL (lread#40)
…tations * upstream/main: dev: lint tweaks (lread#41)
We now know know that an eval expectation is complete when its form parses successfully. Added the lovely nubank matcher combinators dep to help with test assertions. Now that things are a bit more complicated, algorithm and naming in code. It might be a bit easier to follow now? Hope so.
|
Ok, seems to pass on all known projects now (test-doc-blocks, honeysql, rewrite-clj, etaoin, oksa). I've got one or two more things I'd like to check before merging. If you have any feedback on my changes, I am happy to hear it! |
mattiuusitalo
left a comment
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.
Nice! Everything works in the tests I get from malli readme's too
|
There is an issue with embedded assertion tokens for |
We now would only ever need the single semicolon distinguisher for `=stdout=>` and `=stderr=>` blocks.
|
@mattiuusitalo I did one more verification. Looks good! The only change I see in the diff is your fix from #29. Shall I merge to main and cut a release? |
|
I think we are good to go! My use cases work as well. Nice addition with that regression test. |
Fixes #33
Allows both old style and new style
Looking at the example above one can see the value of leading comments, as the examples written like that are rendered more nicely.