Indent messages correctly in DocumentationFormatter. #2649

ioquatix · 2019-07-13T05:41:50Z

When using example.reporter.message, it would be nice if the messages were indented correctly, so that it's clear it relates to the current spec being executed.

benoittgt · 2019-07-13T15:59:20Z

Hello Samuel and thanks for this PR.

Don't you think we should test this?

Cheers

JonRowe · 2019-07-13T20:44:10Z

Thanks Samuel! Yep I'd like to see a test for this and an indication of the change to the output as a sample!

ioquatix · 2019-07-13T22:13:16Z

I am happy to add a test. However there is no current test for this.

If we add a test, we are setting in stone how it should behave. Is that okay? Maybe we want to change this behaviour again in the future.

ioquatix · 2019-07-13T23:01:45Z

Okay, I've added a spec.

I don't want to make this any more complicated, however it does make me wonder if the message should come after the example name. While right now we are retaining the current behaviour which is probably good, maybe there is another discussion to be had regarding how to output the messages.

benoittgt · 2019-07-14T09:53:43Z

If we add a test, we are setting in stone how it should behave. Is that okay? Maybe we want to change this behaviour again in the future.

Then we will change the test :) No problem for me.

maybe there is another discussion to be had regarding how to output the messages.

Yes maybe. I think it's ok for a first version.

benoittgt

LGTM thanks Samuel

spec/rspec/core/formatters/documentation_formatter_spec.rb

ioquatix · 2019-07-15T04:06:52Z

Are we good to merge?

lib/rspec/core/formatters/documentation_formatter.rb

ioquatix · 2019-07-15T08:13:50Z

Sorry, missed adding it in #finished.

benoittgt · 2019-07-15T08:14:41Z

I much more happy with the new expected output now.

ioquatix · 2019-07-15T08:17:00Z

I agree, it's nicer, it's just a bigger change and affects the underlying assumptions about what message output does. I wasn't so confident to make a big change.

ioquatix · 2019-07-15T08:22:47Z

How can a developer, at a glance tell which example it relates to?

I think the main criteria you should consider is, give that currently it is:

root
  example1
message
  example2

Is it already so hard to read that

root
  example1
  example2
    message

couldn't really make it any worse than it already is, then merging this PR only makes the situation better, and no worse for any existing use case.

JonRowe · 2019-07-15T08:24:19Z

My comment referred to the original implementation, not the one I've suggested as being mergeable.

ioquatix · 2019-07-15T08:26:35Z

My comment referred to the original implementation, not the one I've suggested as being mergeable.

Okay, fair enough, well I've implemented what you asked but travis has exploded, strangely enough all tests passing locally, not sure what's going on.

JonRowe · 2019-07-15T08:28:16Z

The cucumber is failing because message is used to report messages before the suite too, so oddly, I think there needs to be another flush before the examples run

ioquatix · 2019-07-15T08:29:19Z

@JonRowe why is it not failing for me locally, do I need to install cucumber?

JonRowe · 2019-07-15T08:30:00Z

cucumber is installed if you've done a bundle install, but you need to run it, its not part of our spec suite

ioquatix · 2019-07-15T08:30:38Z

@JonRowe

Is this what you are thinking?

ioquatix · 2019-07-15T08:31:39Z

rake cucumber?

JonRowe · 2019-07-15T08:41:24Z

That should be one way

ioquatix · 2019-07-15T08:43:11Z

I tried it but it still fails.

ioquatix · 2019-07-15T08:43:44Z

How about this?

        def message(notification)
          @messages << notification.message

          flush_messages if @group_level.zero?
        end

ioquatix · 2019-07-15T09:23:10Z

@JonRowe Okay it's implemented and ready for your review.

ioquatix · 2019-07-16T08:58:23Z

@JonRowe any chance we can merge this? Everything you requested was implemented.

JonRowe

Sorry to be so nitpicky @ioquatix, I think we should change this slightly. If we keep track of wether we are in an example (causing the buffering to happen) but print immediately or not I feel the behaviour is cleaner. It means that outside of an example (groups, setup etc) its immediately printed at the current indent level, but if an example is running its buffered and indented further.

lib/rspec/core/formatters/documentation_formatter.rb

ioquatix · 2019-08-28T01:59:00Z

@JonRowe are you happy with this now?

ioquatix · 2019-09-06T22:32:45Z

@JonRowe Any update?

spec/rspec/core/formatters/documentation_formatter_spec.rb

benoittgt · 2019-09-09T09:59:07Z

Sorry for the delay. Actual code looks good to me. Thanks Samuel

spec/rspec/core/formatters/documentation_formatter_spec.rb

lib/rspec/core/formatters/documentation_formatter.rb

JonRowe · 2019-09-09T15:49:36Z

Sorry for the delay, lots on my plate atm

ioquatix · 2019-09-09T22:38:50Z

@JonRowe I made a few minor changes to your suggestions.

I prefered @example_started which matches the name of the method.
I didn't put the state transition @example_started = false into flush_messages as I felt like users of flush_messages might get confused that it also changes the state of the example buffering. i.e. maybe it gets called more than once per example or something else.

Is there any chance example_started gets called more than once without example_passed or example_failed being called?

Also, do we need @example_started = false in example_pending?

JonRowe · 2019-09-10T09:45:12Z

lib/rspec/core/formatters/documentation_formatter.rb

                            :example_passed, :example_pending, :example_failed

        def initialize(output)
          super
          @group_level = 0
+
+          @example_started = false


Its not the name of the method, its the state of the example, I prefer running as it indicated the state now, not what it was.

JonRowe · 2019-09-10T09:46:01Z

lib/rspec/core/formatters/documentation_formatter.rb

        end

      private

+        def flush_messages
+          if @messages.any?


You don't need to check for this, you can just iterate and clear, its a no op on an empty array.

JonRowe · 2019-09-10T09:47:06Z

Is there any chance example_started gets called more than once without example_passed or example_failed being called?

Also, do we need @example_started = false in example_pending?

Example started is called once at the start of an example, one of the three result methods should be called always so yes you need it in pending.

ioquatix · 2019-09-10T10:04:13Z

Okay changes made.

JonRowe · 2019-09-10T10:05:09Z

Will merge when green

ioquatix · 2019-09-10T10:19:20Z

Awesome, look forward to seeing this in my specs :)

ioquatix · 2019-09-10T10:20:51Z

Also thanks for all your feedback, and doubly so if you've been preoccupied with something else!

benoittgt · 2020-01-12T14:34:50Z

Just a notice. Samuel did a video while working on this PR. https://youtu.be/tj4Bz_HaDIk 👌

Indent messages correctly in DocumentationFormatter.

Indent messages correctly in DocumentationFormatter. --- This commit was imported from rspec/rspec-core@e570a0c.

This commit was imported from rspec/rspec-core@25b10ef.

benoittgt approved these changes Jul 14, 2019

View reviewed changes

JonRowe reviewed Jul 14, 2019

View reviewed changes

spec/rspec/core/formatters/documentation_formatter_spec.rb Outdated Show resolved Hide resolved

JonRowe suggested changes Jul 15, 2019

View reviewed changes

lib/rspec/core/formatters/documentation_formatter.rb Outdated Show resolved Hide resolved

benoittgt self-requested a review July 15, 2019 08:11

ioquatix force-pushed the patch-1 branch from 0266427 to 93767e9 Compare July 15, 2019 08:13

ioquatix force-pushed the patch-1 branch from 93767e9 to 7143703 Compare July 15, 2019 09:22

benoittgt approved these changes Jul 15, 2019

View reviewed changes

JonRowe suggested changes Jul 17, 2019

View reviewed changes

lib/rspec/core/formatters/documentation_formatter.rb Outdated Show resolved Hide resolved

benoittgt reviewed Sep 9, 2019

View reviewed changes

spec/rspec/core/formatters/documentation_formatter_spec.rb Outdated Show resolved Hide resolved

JonRowe suggested changes Sep 9, 2019

View reviewed changes

spec/rspec/core/formatters/documentation_formatter_spec.rb Outdated Show resolved Hide resolved

JonRowe reviewed Sep 9, 2019

View reviewed changes

lib/rspec/core/formatters/documentation_formatter.rb Outdated Show resolved Hide resolved

JonRowe reviewed Sep 9, 2019

View reviewed changes

lib/rspec/core/formatters/documentation_formatter.rb Outdated Show resolved Hide resolved

JonRowe reviewed Sep 9, 2019

View reviewed changes

lib/rspec/core/formatters/documentation_formatter.rb Show resolved Hide resolved

JonRowe reviewed Sep 9, 2019

View reviewed changes

lib/rspec/core/formatters/documentation_formatter.rb Outdated Show resolved Hide resolved

ioquatix added 2 commits September 10, 2019 10:34

Don't indent messages which are not buffered.

fc242aa

Prefer to reuse array, track example state separately.

e2171dc

JonRowe suggested changes Sep 10, 2019

View reviewed changes

ioquatix added 2 commits September 10, 2019 22:02

Rename @example_started to @example_running.

a99d6d5

Remove conditional around flushing messages.

0022831

JonRowe approved these changes Sep 10, 2019

View reviewed changes

JonRowe merged commit e570a0c into rspec:master Sep 10, 2019

JonRowe added a commit that referenced this pull request Sep 10, 2019

Changlog for #2649

25b10ef

ioquatix deleted the patch-1 branch September 10, 2019 10:44

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#2649 from ioquatix/patch-1

9618434

Indent messages correctly in DocumentationFormatter.

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Changlog for rspec#2649

e44cd55

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[core] Changlog for rspec/rspec-core#2649

98c2c5d

This commit was imported from rspec/rspec-core@25b10ef.

Uh oh!

Indent messages correctly in DocumentationFormatter. #2649

Indent messages correctly in DocumentationFormatter. #2649

Uh oh!

Conversation

ioquatix commented Jul 13, 2019

Uh oh!

benoittgt commented Jul 13, 2019

Uh oh!

JonRowe commented Jul 13, 2019

Uh oh!

ioquatix commented Jul 13, 2019

Uh oh!

ioquatix commented Jul 13, 2019

Uh oh!

benoittgt commented Jul 14, 2019

Uh oh!

benoittgt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

benoittgt commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonRowe commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

JonRowe commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

JonRowe commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

JonRowe commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 15, 2019

Uh oh!

ioquatix commented Jul 16, 2019

Uh oh!

JonRowe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ioquatix commented Aug 28, 2019

Uh oh!

ioquatix commented Sep 6, 2019

Uh oh!

Uh oh!

benoittgt commented Sep 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonRowe commented Sep 9, 2019

Uh oh!

ioquatix commented Sep 9, 2019

Uh oh!

JonRowe Sep 10, 2019

Choose a reason for hiding this comment

benoittgt commented Jul 15, 2019 •

edited

Loading

ioquatix commented Jul 15, 2019 •

edited

Loading