Skip to content
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

cider-test: don't render a newline between expected and actual #3375

Merged
merged 5 commits into from
Aug 4, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Jul 21, 2023

Before:

Screen Shot 2023-07-21 at 17 57 03

After:

Screen Shot 2023-07-21 at 17 56 30

Personally I just didn't like this newline. For large reports it can make quite a difference.

Cheers - V

@vemv vemv requested a review from bbatsov July 21, 2023 16:00
@bbatsov
Copy link
Member

bbatsov commented Jul 22, 2023

For short results it looks better without the newline, but I wondering if it helps readability for multi-line results. Let me think a bit about this.

@vemv
Copy link
Member Author

vemv commented Jul 26, 2023

Now that diff is conditionally present, maybe our heuristic can be:

  • if there's a diff, consider that the expected/values are complex enough, and therefore deserve a newline between them
  • else, no newline

@vemv
Copy link
Member Author

vemv commented Jul 26, 2023

Now the newline is inserted conditionally, following my previous comment.

@aisamu
Copy link

aisamu commented Jul 26, 2023

NB: Commenting because bbatsov asked on slack

I also found no whitespace to look better for short results, as it's easier to eyeball differences that way.

Perhaps diff would benefit from always having whitespace (between itself and actual, that is): the diff is self-contained and I don't see myself comparing it back-and-forth against actual. It's also large and makes the usual expected vs actual back-and-forth a bit harder/noisier than the no-diff case.

Would it be possible to post a screenshot of a multiline output as well, @vemv?

@vemv
Copy link
Member Author

vemv commented Jul 26, 2023

Good input, much appreciated!

So after that, the logic is simple and visually consistent:

  • if there's a diff, there are newlines between expected/actual/diff
  • if there's not a diff, there are no newlines between expected/actual

A screenshot showing both paths:

image

@aisamu
Copy link

aisamu commented Jul 26, 2023

That indeed looks nicer!

Just to clarify what I originally meant: we could also always have whitespace between actual and diff, and, orthogonally, only add a newline between expected and actual when they're multi-line.

More concretely, your example would then be:

expected: {1 (0 1), 3 (0 1 2 3), 5 (0 1 2 3 4 5)}
  actual: {5 (0 1 2 3 21 5), 3 (0 1 2 3), 1 (0 1)}

    diff: - {5 [nil nil nil nil 47}
          + {5 [nil nil nil nil 217}

That way we'd still benefit from the easier eyeballing of actual vs expected without the diff output interfering.
(Since the diff is "self-contained", I'm assuming it relies less on back-and-forths.)

But, as bbatsov's mentions, I'm unsure how the lack of whitespace between actual and expected would play out when expected/actual are multi-line.

@bbatsov
Copy link
Member

bbatsov commented Jul 27, 2023

I meant the same @aisamu, but I don't feel strongly about this.

Copy link
Member

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can always change this down the road.

@bbatsov
Copy link
Member

bbatsov commented Aug 3, 2023

@vemv Just add some changelog entry and this one is good to go.

@vemv
Copy link
Member Author

vemv commented Aug 4, 2023

Makes sense @aisamu - implemented!

image image image image

@vemv vemv merged commit 1dc2c80 into master Aug 4, 2023
20 checks passed
@vemv vemv deleted the expected-no-nl branch August 4, 2023 05:59
(let ((trimmed-string (replace-regexp-in-string "\n\\'" "" input-string)))
(and (string-match-p "\n" trimmed-string)
t))))
(and (string-match-p "\\n" input-string)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this regexp really match?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaped newlines as coming from Clojure / cider-nrepl. These aren't vanilla elisp newlines.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It matches the letter "n" in a string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! will fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants