style: reformat and restructure tests in parser_test.clj

[roryokane]

See commit messages for details.

[jeroenvandijk]

I'm not approving it, because of the comma's. Personally I don't think that's an improvement

[jeroenvandijk]

I think this is an improvement

[jeroenvandijk]

I actually hate the commas. That's personal, so feel free to disagree :) If you want to make it more readable for me, I think putting it on the same line, or when that doesn't fit using newlines between statements, is better

[roryokane]

I agree the commas look kind of ugly, but I thought they were the best of four bad options. Let me show you the options I considered so you can compare and choose the one you like most:

The following style was confusing because among the 10 similar lines, I found it hard to tell which line was the expected value and which was the input. I felt like I had to manually count from the top and remember which was even and which was odd.

    ["some text" [:link] "around a link"]
    ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39]
    [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

If I put each expected value and input on the same line like the following code, I find it hard to see where one vector ends and the next begins, even with rainbow parens:

    ["some text" [:link] "around a link"] ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39] [{:something nil} "more" " " "text" [:link] "between" " " 

The following looks nice to me, but cljstyle doesn’t accept it. I can understand that it complains because it doesn’t reflect the structure of the forms, even though I know it reflects their semantics. One option would be using this and ignoring this file, but that could ignore other style problems in this file that should be caught, so I don’t like that option.

    ["some text" [:link] "around a link"]
      ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39]
      [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

Since I liked the above but cljstyle didn’t, I added the commas to make it pass, resulting in the formatting in this PR:

    ["some text" [:link] "around a link"]
    , ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39]
    , [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

Those are the options I thought of before, but now I’ve thought of a few more:

It seems cljstyle doesn’t mind if I put extra spaces between the expected value and input, as follows. It’s better than the single space version, at least. But I like putting the expected value and input on separate lines because it makes it easier to see what differences, if any, they have. Doing a visual diff when they are vertically close is easy for all the tests in this file because they all transform the input from left to right, so the corresponding parts of each value stay visually close.

    ["some text" [:link] "around a link"]   ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39]   [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

I could also put a blank line between every expected-actual pair, as follows. This makes every line unambiguous but requires more scrolling when viewing the file in the editor.

    ["some text" [:link] "around a link"]
    ["some" " " "text" [:link] "around " "a link"]
    
    [{:something nil} "more text" [:link] "between elements" 39]
    [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]
    

Or I could try marking the lines with comments somehow, like as follows. I think it’s not that readable though, as the comments are at the end of each line. It’s hard to notice the comments with my syntax highlighting, so I have to scan to the end of each line to see them.

    ["some text" [:link] "around a link"] ;; key
    ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39] ;; key
    [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

Adding commas at the end of key lines, as follows, has the same problem:

    ["some text" [:link] "around a link"],
    ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39],
    [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

Those are the options I can think of now. Which of these do you prefer? If you still hate the commas-before-inputs style I think my next favorite one is the blank lines style.

I just realized that the blank lines style might be what you meant by “using newlines between statements”. Is that what you meant?

[jeroenvandijk]

@roryokane Thanks for putting so much time into explaining it!

Maybe we can tweak cljstyle where it is biting us (maybe a specific config for are. I haven't tried this yet).

For me, the comma's are not helpful for understanding the code. It mostly noise for me, just like how the clojure reader treats it. At the same time though accepting this style forces me to think about putting the comma's in the right spot and this puts me back into the json world. So I strongly prefer the blank lines to distinguish examples (or categories).

I just looked up what clojure.core does. They do something similar in their tests

[tangjeff0]

Yeah, I like how it's done in clojure.core. Keep commas out of Clojure!!! Can we tweak the linter config?

[roryokane]

With the help of @jelmerderonde’s comment about tweaking cljstyle’s config, I configured cljstyle to automatically indent every second value in are forms. Do you find that style acceptable?

    ["some text" [:link] "around a link"]
      ["some" " " "text" [:link] "around " "a link"]
    [{:something nil} "more text" [:link] "between elements" 39]
      [{:something nil} "more" " " "text" [:link] "between" " " "elements" 39]

[roryokane]

Whoops, there’s one test file I overlooked, test/athens/lib/dom/attributes_test.cljc, that uses are with three variables. The alternating stair indent style isn’t appropriate for that code, yet cljstyle formats it because it uses are. I need to figure out a way to narrow the scope of the config indent rule.

[jeroenvandijk]

@roryokane what do you think about just add one blank line in between like in test/athens/lib/dom/attributes_test.cljc? I think that's readable enough?

[jeroenvandijk]

Thanks @roryokane!