Use a custom diff view for binary files by Toilal · Pull Request #737 · yeoman/generator

Toilal · 2015-01-17T09:14:51Z

Here the binary diff discussed in #736

Here's the overview.

 conflict gradle-wrapper.jar
? Overwrite gradle-wrapper.jar? show the differences between the old and the new
┌───────────────┬──────────────────────────┬─────────────┬──────┐
│               │ Existing                 │ Replacement │ Diff │
├───────────────┼──────────────────────────┼─────────────┼──────┤
│ Size          │ 51.01 kB                 │ 51.01 kB    │ -2 B │
├───────────────┼──────────────────────────┼─────────────┼──────┤
│ Last modified │ Sat Jan 17 2015 10:03:33 │ N/A         │ N/A  │
└───────────────┴──────────────────────────┴─────────────┴──────┘
? Overwrite gradle-wrapper.jar? (Ynaxdh)

coveralls · 2015-01-17T09:16:54Z

Coverage decreased (-0.52%) when pulling 6ed97bb on Toilal:feat/binary-diff into 03c0ca3 on yeoman:master.

sindresorhus · 2015-01-17T09:33:16Z

Nice work @Toilal :)

mtime => Last modified

I wonder if it would make more sense to inverse the table, so Size, Modified is on the left, and Existing, Future, Diff is on the top? Thoughts?

sindresorhus · 2015-01-17T09:34:44Z

package.json

We already depend on isbinaryfile, maybe reuse that?

Ho yes, of course, i didn't see there was already this dependency.

Problem is that isbinaryfile needs a file as input, and we only have a buffer in conflicter.js ... This could be refactored to have a file, but i'm not sure it make sense ...

@Toilal Yeah, I guess it doesn't matter much dep wise. My only concern is that this one requires us to read in the whole file and then detect whether it's binary. isbinaryfile only read the required bytes and should be much faster.

I would agre but file is fully read just after using isbinaryfile.

@see #737 (comment).

@Toilal I don't know how the check works with that module. Maybe it needs to read until it hits some kind of needle. I haven't read their code.

Currently, isbinaryfile is loads 512 first bytes in a buffer, and then make the check with this buffer. istextorbinary has a buffer as input, so it's up to the user to load the buffer, and it then reads the buffer until it reach a binary byte.

So it seems that using only 512 bytes of data could be enough, and would make isbinaryfile and istextorbinary quite equivalent ...

Alright, then go for it.

As the conflicter handles buffers (he doesn't know of the actual template file), I'd drop isbinaryfileand only use istextorbinary.

coveralls · 2015-01-17T10:40:23Z

Coverage decreased (-0.52%) when pulling 0cd1d60 on Toilal:feat/binary-diff into 03c0ca3 on yeoman:master.

Toilal · 2015-01-17T10:44:20Z

I've reverse the table ordering.

I've kept istextorbinary because it handle buffer as input, where isbinaryfile handle file as input.

Currently, isbinaryfile is only used in actions.js.

  var binary = isBinaryFile(source);
  if (!binary) {
    encoding = 'utf8';
  }

  body = fs.readFileSync(source, encoding);

Could be replaced using istextorbinary

  body = fs.readFileSync(source);
  if (istextorbinary.isTextSync(body)) {
    body = body.toString();
  }

It would drop one dependency, and avoid the file to be read 2 times. I'll make another PR for this one.

Toilal · 2015-01-17T10:56:26Z

Another question : Should i use the extension feature of istextorbinary ?

It can use the filename extension to check if it's a binary file, and it will then check the actual content of buffer if extension is unknown only.

coveralls · 2015-01-17T11:06:41Z

Coverage decreased (-0.52%) when pulling 9958860 on Toilal:feat/binary-diff into 03c0ca3 on yeoman:master.

coveralls · 2015-01-17T12:43:14Z

Coverage decreased (-1.21%) when pulling e4b09fd on Toilal:feat/binary-diff into 03c0ca3 on yeoman:master.

sindresorhus · 2015-01-17T12:53:58Z

lib/util/conflicter.js

Just use read-chunk

Maybe you could merge and release this one then : sindresorhus/read-chunk#2

Toilal · 2015-01-17T13:55:47Z

Should be OK now, using read-chunk 1.0.1.

coveralls · 2015-01-17T13:55:55Z

Coverage decreased (-0.6%) when pulling 82177ce on Toilal:feat/binary-diff into 03c0ca3 on yeoman:master.

SBoudrias · 2015-01-17T19:33:41Z

lib/util/conflicter.js

Move the content of the if statement to a helper function (e.g. diffBinary). It'll be easier to unit test, and easier to read.

SBoudrias · 2015-01-17T19:37:00Z

We can do it after this is merged, but we should get all the logic around generating a diff console output to a standalone module.

(Also, we want to stop using adapter.diff() method)

Toilal · 2015-01-17T21:46:34Z

OK i've pushed a new version with your comments related changes @SBoudrias

coveralls · 2015-01-17T21:48:18Z

Coverage decreased (-1.11%) when pulling 659d083 on Toilal:feat/binary-diff into 45bb7a5 on yeoman:master.

coveralls · 2015-01-17T21:55:08Z

Coverage decreased (-1.2%) when pulling 3238b09 on Toilal:feat/binary-diff into 45bb7a5 on yeoman:master.

Toilal · 2015-01-18T07:26:14Z

I'll rebase

coveralls · 2015-01-18T07:30:35Z

Coverage decreased (-1.2%) when pulling f71de02 on Toilal:feat/binary-diff into 697ecb2 on yeoman:master.

SBoudrias · 2015-01-18T07:32:22Z

LGTM, I'll let @sindresorhus go over it one last time.

sindresorhus · 2015-01-18T07:36:20Z

lib/util/binary-diff.js

Why is this N/A?

The new file is still not created

But maybe blank is better ?

We only pass in the vinyl file.contents here, but if we were to pass in the file instead we could get the stat: dateFormat(newFile.stat.mtime)

I agree, but it would change the conflicter checkForCollision function signature. Isn't this part of the API ?

If no problem, I can do the refactor.

I'm not following. @SBoudrias

But isn't it just about passing file instead of file.contents here https://github.com/yeoman/generator/pull/737/files#diff-0765c740f9a715d47c62655d99f00ee6R171 ?

Only watching this line may confuse ...

You have to read the real structure of file object, which is not a Vinyl file, here : https://github.com/Toilal/generator/blob/feat/binary-diff/lib/util/conflicter.js#L42-L45

file.path is the destination filepath (this is the existing file), and file.contents is the replacement content of the file (this is the new file).

sindresorhus · 2015-01-18T07:43:07Z

I would also like to see a test if possible.

Toilal · 2015-01-20T15:17:43Z

I've written test checking display of right diff with binary and text files.

I'm quite new in javascript, so I hope the mocks are correctly written using sinon.

coveralls · 2015-01-20T15:23:20Z

Coverage decreased (-1.2%) when pulling 9693eea on Toilal:feat/binary-diff into b611741 on yeoman:master.

kevva · 2015-01-20T15:38:12Z

Add .idea to your global .gitignore (in HOME).

Toilal · 2015-01-20T15:42:18Z

Sorry about that, and thanks for the tip.

coveralls · 2015-01-20T15:45:05Z

Coverage increased (+0.15%) when pulling a7223ae on Toilal:feat/binary-diff into b611741 on yeoman:master.

coveralls · 2015-01-20T15:51:41Z

Coverage increased (+0.15%) when pulling a7223ae on Toilal:feat/binary-diff into b611741 on yeoman:master.

SBoudrias · 2015-01-21T07:11:00Z

lib/test/adapter.js

Why did you changed that file?

These change are irrelevant to the current matter, and setup method that mutate the object state are pretty hard to maintain on the long run. Please revert those changes

Yes sorry. It was used at some time in unit tests, but not required anymore.

coveralls · 2015-01-21T07:19:54Z

Coverage increased (+0.15%) when pulling 489db88 on Toilal:feat/binary-diff into b611741 on yeoman:master.

SBoudrias · 2015-01-25T22:52:50Z

Alright, this LGTM! Thanks for the work you done on this one!!

Use a custom diff view for binary files

sindresorhus · 2015-01-26T06:36:03Z

👍 Awesome work on this one @Toilal 💃

sindresorhus reviewed Jan 17, 2015
View reviewed changes

Toilal force-pushed the feat/binary-diff branch from 6ed97bb to 0cd1d60 Compare January 17, 2015 10:38

Toilal force-pushed the feat/binary-diff branch from 0cd1d60 to 9958860 Compare January 17, 2015 11:04

Toilal mentioned this pull request Jan 17, 2015

Replace isbinaryfile with istextorbinary #738

Merged

Toilal force-pushed the feat/binary-diff branch from 9958860 to e4b09fd Compare January 17, 2015 12:40

sindresorhus reviewed Jan 17, 2015
View reviewed changes

Toilal force-pushed the feat/binary-diff branch from e4b09fd to 82177ce Compare January 17, 2015 13:53

SBoudrias reviewed Jan 17, 2015
View reviewed changes

Toilal force-pushed the feat/binary-diff branch from 82177ce to 659d083 Compare January 17, 2015 21:45

Toilal force-pushed the feat/binary-diff branch from 659d083 to 3238b09 Compare January 17, 2015 21:52

Toilal force-pushed the feat/binary-diff branch from 3238b09 to f71de02 Compare January 18, 2015 07:28

sindresorhus reviewed Jan 18, 2015
View reviewed changes

Toilal force-pushed the feat/binary-diff branch from f71de02 to 9693eea Compare January 20, 2015 15:15

Toilal force-pushed the feat/binary-diff branch 2 times, most recently from a409621 to 0f91850 Compare January 20, 2015 15:34

Toilal force-pushed the feat/binary-diff branch from 0f91850 to a7223ae Compare January 20, 2015 15:41

SBoudrias reviewed Jan 21, 2015
View reviewed changes

Use a custom diff view for binary files

489db88

Toilal force-pushed the feat/binary-diff branch from a7223ae to 489db88 Compare January 21, 2015 07:18

SBoudrias added a commit that referenced this pull request Jan 25, 2015

Merge pull request #737 from Toilal/feat/binary-diff

79e882e

Use a custom diff view for binary files

SBoudrias merged commit 79e882e into yeoman:master Jan 25, 2015

Uh oh!

Conversation

Toilal commented Jan 17, 2015

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

sindresorhus commented Jan 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

Toilal commented Jan 17, 2015

Uh oh!

Toilal commented Jan 17, 2015

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toilal commented Jan 17, 2015

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SBoudrias commented Jan 17, 2015

Uh oh!

Toilal commented Jan 17, 2015

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

coveralls commented Jan 17, 2015

Uh oh!

Toilal commented Jan 18, 2015

Uh oh!

coveralls commented Jan 18, 2015

Uh oh!

SBoudrias commented Jan 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Jan 18, 2015

Uh oh!

Toilal commented Jan 20, 2015

Uh oh!