Php diff 120 #128
Php diff 120 #128
Conversation
|
Hey, Of course I'm willing to do a review for you, but you'll have to grant me some time. Aside from work, I'm also active in some other projects which require my attention. |
|
You don't need to worry about time. Do it whenever you find time. |
There was a problem hiding this comment.
Assumming you'll still need to cleanup/format the code, but...
- The files (ex. the eol resources) have an incorrect line seperator.
- Docblocks are missing or unfinshed.
- Since the endings don't change after the file is loaded, you could execute the different methods immediately after loading a file, store the values as properties and use getters for these properties.
Also, I left in-file comments.
lib/jblond/Diff/File.php
Outdated
| { | ||
| $lastLine = $this->getLastLine(); | ||
| if (strpos($lastLine, "\r\n") !== false) { | ||
| return "EOL type is Windows (CRLF)"; |
There was a problem hiding this comment.
Double quotes can be replaced by single quotes
There was a problem hiding this comment.
it can't be replaced. if I replace it with single quotes the unit test fails.
There was a problem hiding this comment.
The comment is referring to line 72, not 71.
lib/jblond/Diff/File.php
Outdated
| } | ||
|
|
||
| if (strpos($lastLine, "\r") !== false) { | ||
| return "EOL type is Mac (CR)"; |
There was a problem hiding this comment.
Double quotes can be replaced by single quotes
There was a problem hiding this comment.
it can't be replaced. if I replace it with single quotes the unit test fails.
There was a problem hiding this comment.
The comment is referring to line 76, not 75.
lib/jblond/Diff/File.php
Outdated
| } | ||
|
|
||
| if (strpos($lastLine, "\n") !== false) { | ||
| return "EOL type is Unix (LF)"; |
There was a problem hiding this comment.
Double quotes can be replaced by single quotes
There was a problem hiding this comment.
it can't be replaced. if I replace it with single quotes the unit test fails.
There was a problem hiding this comment.
The comment is referring to line 80, not 79.
|
Also, If I'm not mistaken, I notice now, the line separators ( |
No description provided.