Extract expectations differ to support #36

phiggins · 2014-01-29T04:44:45Z

This was done to support rspec/rspec-mocks#434 in conjunction with rspec/rspec-expectations#441.

There's still a few things I need to do, but I wanted to get this out there.

JonRowe · 2014-02-01T10:05:55Z

lib/rspec/support/diff_presenter.rb

+      attr_reader :color
+      alias_method :color?, :color
+
+      def initialize(opts={})


I'd prefer it if we made these explicit rather than a hash...

Additional thought, given the extra methods required, perhaps we could have a Differ and ColouredDiffer...

Also traditionally initialize is at the top of the class...

For clarity, you mean something like this:

def initialize(stringifier=nil, color=nil) @stringifier = stringifier || lambda {|string| string }) @color = color || false end

?

Actually I think if stringify must be a block let's do

def initialize(colour = false, &block) @colour = colour @stringifier = block || proc { |string| string } end

Although it's not clear to me what stringifier does?

You'd still want a default though, right? Unless I misunderstand, I'm not sure how that would reduce complexity.

It reduces the work initialize does and in my eyes might make things look nicer, there is also a minor performance cost capturing a block when we don't need it. See L#169 for my suggestion...

Honestly, I prefer the options has over the ordered arguments. There isn't an obvious natural ordering. (Compare that to something like Date.new(2014, 2, 4) where the ordering of the arguments is extremely obvious). From the caller, Differ.new(:color => true) is preferrable than Differ.new(true) -- for the latter, it's unclear what true means. Furthermore, these two values are options (not data members like what you get with a struct or value object) and these just happen to be the two currently supported options. In the future we may gain more. An options hash sets us up better for the future as it will easily flex to support new options. This won't:

def initialize(colour = false, &block) end

Perhaps instead of being a lambda or block it should be a method, with the idea that Differ can be subclassed?

I'm 👎 on this idea. I think subclassing is a fine technique when it's localized within a single code base, but it leads to the brittle base class problem when it crosses code bases (i.e. base class in one code base, subclass in another). We're running into this in rspec-core where @JonRowe is working on updating the formatter classes but we have to be uber careful not to break existing formatters that subclass formatter base classes.

I much prefer the composition approach of passing the lambda option.

Honestly, I prefer the options has over the ordered arguments.

I do too.

I much prefer the composition approach of passing the lambda option.

Agreed, I was trying to brainstorm ideas.

JonRowe · 2014-02-01T10:10:35Z

@myronmarston will want to review this further, but I made a start :) Thanks for helping with this @phiggins

JonRowe · 2014-02-01T10:11:33Z

Travis reckons you have a syntax error too ;)

phiggins · 2014-02-01T23:31:30Z

Travis reckons you have a syntax error too ;)

Yikes, I was running the tests locally, I'm not sure what happened...

JonRowe · 2014-02-02T20:48:41Z

lib/rspec/support/diff_presenter.rb

+      end
+
+      def object_to_string(object)
+        object = @stringifier.call(object)


(Follow up to above), how about changing this to object = yield(object) if block_given? and passing in the stringifier if it's needed

Honestly, I prefer this how it is. Usually blocks (in a case like Differ.new { }) will always (or nearly always) be used but here the more common case is probably that it's not used. It's unclear at the call site what the block does or represents. As this grows there may be additional lambda options needed, and using a block for the object prep gives it a place of prominence that I don't think makes sense relative to its role here.

myronmarston · 2014-02-04T16:00:10Z

This is looking good to me. That said, it's really hard for me to judge if you properly ported everything from rspec-expectations, including the recent changes after you started working on this. Is there a simple command I can use to diff what you have here against the corresponding files in rspec-expectations master to see the diff from there?

phiggins · 2014-02-05T15:09:34Z

Is there a simple command I can use to diff what you have here against the corresponding files in rspec-expectations master to see the diff from there?

I tried diffing the changes made after I extracted the code from expectations to support and gave up after a few minutes, it is either beyond me or not easily done with git.

I could make the cleanup changes to expectations, get those accepted, and then extract to support. This would be a much cleaner diff but would require some not insignificant rework on my part.

JonRowe · 2014-02-05T21:15:12Z

Use a normal differ :) (diff file_1 file_2 on osx...), if you upload the results to a gist I think it'll colour it up for you.

phiggins · 2014-03-24T06:47:05Z

Here's a diff of the diff_presenter.rb from rspec-expectations's master and my branch of rspec-support: https://gist.github.com/phiggins/9735292

Sorry this fell off my radar, getting a new job will do that to you! I'd like to get this closed out soon just so it's not hanging around in limbo.

JonRowe · 2014-03-24T09:15:47Z

This also needs rebasing I'm afraid.

JonRowe · 2014-03-24T09:16:08Z

lib/rspec/support/differ.rb

@@ -0,0 +1,204 @@
+require "rspec/support/encoded_string"
+require "rspec/support/hunk_generator"


These should leverage our new require stuff.

Is there somewhere where the "new require stuff" is discussed? I don't keep up with rspec development I'm afraid.

See: https://github.com/rspec/rspec-support/blob/master/lib/rspec/support.rb#L14-29 (especially L29 as that shows a require using the optimised path)

phiggins · 2014-03-24T09:23:42Z

This has been rebased.

JonRowe · 2014-03-24T10:41:35Z

It'd be good to squash this too.

JonRowe · 2014-03-24T10:42:12Z

Also the build is failing, due to a missing require?

myronmarston · 2014-03-24T15:17:58Z

BTW, I think there have been some changes in the original files in rspec-expectations. For example:

rspec/rspec-expectations@859eedf

These sorts of changes will need to be pulled in here.

phiggins · 2014-03-24T23:14:37Z

BTW, I think there have been some changes in the original files in rspec-expectations.

Good catch. I'll make sure to post full diffs of the files I moved when I get all the other feedback sorted out.

phiggins · 2014-03-25T05:53:57Z

It looks like the travis builds are failing because it depends on changes I've made in my expectations branch. Is there a good way to work around this?

myronmarston · 2014-03-25T05:59:54Z

It looks like the travis builds are failing because it depends on changes I've made in my expectations branch. Is there a good way to work around this?

Commit a change to the Gemfile that uses your branch for rspec-expectations. We'll just have to remember to revert it before merging.

JonRowe · 2014-03-25T06:01:13Z

I'm curious to know what changes... This should be independent of rspec-expectations?

phiggins · 2014-03-25T06:02:41Z

Here's the diffs of files I changed:
differ_spec / diff_presenter_spec
Differ / DiffPresenter
EncodedString
encoded_string_spec
HunkGenerator / Differ

I just realized I did the diffs backwards. :/

phiggins · 2014-03-25T06:03:14Z

I'm curious to know what changes

I renamed some classes.

phiggins · 2014-03-25T06:28:41Z

To fix some travis failures in rspec-expectations I added #empty? to the list of methods EncodedString delegates to the underlying string:

phiggins@8bbc264

https://travis-ci.org/rspec/rspec-expectations/jobs/21486966

JonRowe · 2014-03-25T10:17:11Z

I'm curious to know what changes

I renamed some classes.

But that shouldn't be breaking the support build, this should be additional code isolated from expectations.

phiggins · 2014-03-25T16:26:18Z

But that shouldn't be breaking the support build, this should be additional code isolated from expectations.

I renamed Differ to HunkGenerator and DiffPresenter to Differ. rspec-expectations used DiffPresenter. After renaming the class in rspec-support, rspec-support's tests wouldn't run because they use rspec-expectations.

myronmarston · 2014-03-25T16:41:41Z

I renamed Differ to HunkGenerator and DiffPresenter to Differ. rspec-expectations used DiffPresenter. After renaming the class in rspec-support, rspec-support's tests wouldn't run because they use rspec-expectations.

I'd expect them to be namespaced such that using different names for them in rspec-support shouldn't cause any problems for rspec-expectations. e.g. in rspec-expectations they were RSpec::Expectations::Differ and here it's RSpec::Support::HunkGenerator...given that the code here is additive and rspec-expectations hasn't yet been updated to rely on the code in rspec-support, that shouldn't break anything in rspec-expectations (unless I'm missing something).

phiggins · 2014-03-25T16:47:28Z

I think you guys might be correct, that might have been a red herring. I'll poke at it when I have a few minutes.

phiggins · 2014-03-26T06:43:00Z

I removed the dependence on my branch of rspec-expectations and everything seemed to pass except for one strange failure on REE. Except for squashing the commits, is there anything that needs to be done on either PR?

JonRowe · 2014-03-26T10:14:03Z

Yep, assuming this now matches the changes to the differ in expectations, just needs squashing.

phiggins · 2014-03-26T16:16:34Z

Squashed.

…upport Extract expectations differ to support

JonRowe · 2014-03-26T21:34:43Z

Thanks for having the patience to get this across the line @phiggins!

phiggins · 2014-03-26T21:38:20Z

Thanks for your patience while I took a six week hiatus. 🍹

phiggins mentioned this pull request Jan 29, 2014

Extract differ to support rspec/rspec-expectations#441

Merged

JonRowe reviewed Feb 1, 2014
View reviewed changes

JonRowe reviewed Feb 2, 2014
View reviewed changes

JonRowe reviewed Mar 24, 2014
View reviewed changes

Extract diffing code from expectations to support.

da8dcbe

JonRowe added a commit that referenced this pull request Mar 26, 2014

Merge pull request #36 from phiggins/extract-expectations-differ-to-s…

14b723a

…upport Extract expectations differ to support

JonRowe merged commit 14b723a into rspec:master Mar 26, 2014

		@@ -0,0 +1,204 @@
		require "rspec/support/encoded_string"
		require "rspec/support/hunk_generator"

Extract expectations differ to support #36

Extract expectations differ to support #36

Conversation

phiggins commented Jan 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe commented Feb 1, 2014

JonRowe commented Feb 1, 2014

phiggins commented Feb 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myronmarston commented Feb 4, 2014

phiggins commented Feb 5, 2014

JonRowe commented Feb 5, 2014

phiggins commented Mar 24, 2014

JonRowe commented Mar 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phiggins commented Mar 24, 2014

JonRowe commented Mar 24, 2014

JonRowe commented Mar 24, 2014

myronmarston commented Mar 24, 2014

phiggins commented Mar 24, 2014

phiggins commented Mar 25, 2014

myronmarston commented Mar 25, 2014

JonRowe commented Mar 25, 2014

phiggins commented Mar 25, 2014

phiggins commented Mar 25, 2014

phiggins commented Mar 25, 2014

JonRowe commented Mar 25, 2014

phiggins commented Mar 25, 2014

myronmarston commented Mar 25, 2014

phiggins commented Mar 25, 2014

phiggins commented Mar 26, 2014

JonRowe commented Mar 26, 2014

phiggins commented Mar 26, 2014

JonRowe commented Mar 26, 2014

phiggins commented Mar 26, 2014