Skip to content

Fix conflicter by removing toString() call#736

Merged
SBoudrias merged 1 commit intoyeoman:masterfrom
Toilal:fix/conflicter-717
Jan 16, 2015
Merged

Fix conflicter by removing toString() call#736
SBoudrias merged 1 commit intoyeoman:masterfrom
Toilal:fix/conflicter-717

Conversation

@Toilal
Copy link
Contributor

@Toilal Toilal commented Jan 16, 2015

detect-conflict dependency handles buffers properly, so this toString() call is not required anymore and cause false positive on conflicter.

Close #717

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 56484b8 on Toilal:fix/conflicter-717 into 0f1bfa9 on yeoman:master.

@zckrs
Copy link
Member

zckrs commented Jan 16, 2015

👍 fix

@kevva
Copy link
Member

kevva commented Jan 16, 2015

How does the diff look like?

@Toilal
Copy link
Contributor Author

Toilal commented Jan 16, 2015

It just removes a toString() call from file.content buffer before giving it to conflicter ...

@kevva
Copy link
Member

kevva commented Jan 16, 2015

Yeah, I know, but I'm wondering how the diff looks like when diffing two files with the conflicter if one is a buffer.

@Toilal
Copy link
Contributor Author

Toilal commented Jan 16, 2015

@kevva sorry, i thought you could not read the diff in github for some reason :)

In fact it's a good question, because displaying the diff produce an exception ! I'll fix too ...

/home/toilal/.npm/lib/node_modules/yo/node_modules/yeoman-environment/node_modules/inquirer/node_modules/rx/dist/rx.all.js:9229
        throw e;
              ^
TypeError: Object PK��

detect-conflict dependency handles buffers properly, so this toString() call is not required anymore and cause false positive on conflicter.

Close yeoman#717
@Toilal Toilal force-pushed the fix/conflicter-717 branch from 56484b8 to 94b4cf2 Compare January 16, 2015 13:02
@Toilal
Copy link
Contributor Author

Toilal commented Jan 16, 2015

Adding .toString() before calling diff fix the exception. I've check in 0.17.x and diff fails on binary files too.

https://github.com/yeoman/generator/blob/master/lib/util/conflicter.js#L168

That beeing said, does it really make sense to display a diff on binary files ? Maybe we could check if file is binary with istextorbinary, and use a different diff tool for those kind of files if it's revelant ?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 94b4cf2 on Toilal:fix/conflicter-717 into 0f1bfa9 on yeoman:master.

@sindresorhus
Copy link
Member

and use a different diff tool for those kind of files if it's revelant ?

Yes, for binary file we should show file size difference and modified/created time difference.

@Toilal
Copy link
Contributor Author

Toilal commented Jan 16, 2015

Would you like the implementation to be part of this PR ?

@sindresorhus
Copy link
Member

@Toilal Nah, separate PR.

SBoudrias added a commit that referenced this pull request Jan 16, 2015
Fix conflicter by removing toString() call
@SBoudrias SBoudrias merged commit 03c0ca3 into yeoman:master Jan 16, 2015
@SBoudrias
Copy link
Member

Let's follow up with a separate PR fixing diffing for binary file.

Thanks for digging into this!

@Toilal Toilal deleted the fix/conflicter-717 branch January 17, 2015 08:01
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.

[Bug v0.18.x] Conflicter always return true on assets files.

6 participants