Closed roy-tate closed 9 years ago
coveralls
commented
9 years ago Coverage remained the same at 50.78% when pulling 5f0060da5a85c85a3a9883ef6cd71dce4fb8e577 on roy-tate:roy-tate-patch-2 into 2948d5f68b280666319b400b7b4946bd474d5766 on vanstyn:master.
coveralls
commented
9 years ago Coverage remained the same at 50.78% when pulling 5f0060da5a85c85a3a9883ef6cd71dce4fb8e577 on roy-tate:roy-tate-patch-2 into 2948d5f68b280666319b400b7b4946bd474d5766 on vanstyn:master.
vanstyn
commented
9 years ago @roy-tate -
The script does preserve newlines; it keeps \n as \n, and \r\n as \r\n. With your change, the logic that does this has been removed, and files with \r\n are converted to \n. This causes every line of the file to show as changed.
When running the script on the lib dir, like this:
devel/convert_to_moo.pl libYou should be able to do a
git diffand see changes only on certain lines -- the ones which were actually changed by the logic of the script. The problem is that there is some legacy code which is still \r\n, and we don't want to convert them, because then the changes cannot be seen when using git diff.
Also, this is just a one-off dev script (I agree it is a bit ugly but it was written quickly and is not for production use)... It does not yet perform all logic conversions needed to switch from Moose to Moo in its current state. if it ever actually gets ran (for issue #75) it will be ran once, and only once, and then never again... So, I'm not all that concerned with style cleanup
But, thanks for the thought! I do appreciate the effort, and would be more than happy to see other PRs/patches you'd like to send...
roy-tate
commented
9 years ago You are quite correct! I chose this script specifically because it was a "one-off". I will submit a more correct version. I guess the 2 things that bothered me were
I will address those in a version control friendly manner. On Feb 21, 2015 8:09 AM, "Henry Van Styn" notifications@github.com wrote:
@roy-tate https://github.com/roy-tate -
The script does preserve newlines; it keeps \n as \n, and \r\n as \r\n. With your change, the logic that does this has been removed, and files with \r\n are converted to \n. This causes every line of the file to show as changed.
When running the script on the lib dir, like this:
devel/convert_to_moo.pl lib
You should be able to do a
git diff
and see changes only on certain lines -- the ones which were actually changed by the logic of the script. The problem is that there is some legacy code which is still \r\n, and we don't want to convert them, because then the changes cannot be seen when using git diff.
Also, this is just a one-off dev script (I agree it is a bit ugly but it was written quickly and is not for production use)... It does not yet perform all logic conversions needed to switch from Moose to Moo in its current state. if it ever actually gets ran (for issue #75 https://github.com/vanstyn/RapidApp/issues/75) it will be ran once, and only once, and then never again... So, I'm not all that concerned with style cleanup
But, thanks for the thought! I do appreciate the effort, and would be more than happy to see other PRs/patches you'd like to send...
— Reply to this email directly or view it on GitHub https://github.com/vanstyn/RapidApp/pull/123#issuecomment-75373151.
vanstyn
commented
9 years ago Perhaps you are misunderstanding the operation of the script? It already operates correctly... The only reason for the preservation of \r\n is to prevent lines from showing up as changed that are not changed... For lines that we ARE changing we don't need to preserve the newline (and I do not want to). The code should be all \n, but it is a leftover accident from years earlier. As code is changed, I try to replace \r\n with \n, and \t with 2 spaces. The only reason I do not do this in bulk is to preserve the diff history. But, as code gets changed, it should be \n and two-space tab....
So, there is nothing to "fix" in this script regarding the handling of newlines...
But, if you are looking for something to work on, I would love to get some help on any of these items:
roy-tate
commented
9 years ago Yes, I did not understand that you were intentionally converting to \n on a line - by-line basis. I saw it as making a "hybrid" file, and not good. I'll drop this request. I was planning on looking at tests next. I don't understand the project well enough to add features, and would open a ticket for that! On Feb 21, 2015 8:52 AM, "Henry Van Styn" notifications@github.com wrote:
Perhaps you are misunderstanding the operation of the script? It already operates correctly... The only reason for the preservation of \r\n is to prevent lines from showing up as changed that are not changed... For lines that we ARE changing we don't need to preserve the newline (and I do not want to). The code should be all \n, but it is a leftover accident from years earlier. As code is changed, I try to replace \r\n with \n, and \t with 2 spaces. The only reason I do not do this in bulk is to preserve the diff history. But, as code gets changed, it should be \n and two-space tab....
So, there is nothing to "fix" in this script regarding the handling of newlines...
But, if you are looking for something to work on, I would love to get some help on any of these items:
https://github.com/vanstyn/RapidApp/labels/Test%20Coverage
— Reply to this email directly or view it on GitHub https://github.com/vanstyn/RapidApp/pull/123#issuecomment-75374811.
vanstyn
commented
9 years ago @roy-tate -
Feel free to ask me any questions you have. If you are willing to put in time to contribute to the project, I am willing to put in time to help you to be successful... You can reach me most anytime via IRC in #rapidapp on irc.perl.org
The utility does not preserve end-of-line, so this code was removed. Tested with Linux \n and Windows \r\n files. If you intended to preserve line-endings, I can do that instead. The original code preserved line endings unless a line was converted, and the current code always adds native line-endings appropriate for the user's platform via "\n".