Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upFix newline behavior for template rendering on Windows, including tests #570
Conversation
yunyu
force-pushed the
yunyu:fix-template-tests-on-windows
branch
from
ab88b66
to
0353175
yunyu
force-pushed the
yunyu:fix-template-tests-on-windows
branch
2 times, most recently
from
39c250b
to
8dbea67
This fixes tests and template rendering behavior on Windows
yunyu
force-pushed the
yunyu:fix-template-tests-on-windows
branch
3 times, most recently
from
383df18
to
e60484a
yunyu
force-pushed the
yunyu:fix-template-tests-on-windows
branch
2 times, most recently
from
62674a4
to
6f0ef58
yunyu
force-pushed the
yunyu:fix-template-tests-on-windows
branch
3 times, most recently
from
4ea711d
to
53e4ba9
yunyu
force-pushed the
yunyu:fix-template-tests-on-windows
branch
from
53e4ba9
to
c77c915
|
+1 from me |
ivanovdns
commented
Mar 27, 2017
|
We can't change the internal behavior of the readFileToString, either it should be handled in the test side or a extra method should be provided. |
| /* | ||
| Reads from file or classpath | ||
| Reads from file or classpath using the default charset, replacing CRLF line endings with LF ones |
This comment has been minimized.
This comment has been minimized.
pmlopes
Mar 29, 2017
Member
This will be a behavior breaking change and I'm not sure it is correct. One can use the readFileToString to load anything, including, say for example, yaml, json, etc... where in most cases replacing line endings does not introduce side effects, but it could do with some formats.
| return buff.toString(charset); | ||
| } | ||
| int buffLen = buff.length(); | ||
| ByteBuf byteBuf = buff.getByteBuf(); |
This comment has been minimized.
This comment has been minimized.
pmlopes
Mar 29, 2017
Member
What will be the advantage of using the underlying netty API here, IMO we should keep with the vert.x API since it would address eventual changes underlying and later we're just iterating all bytes so one could achieve the same with a loop on the vert.x buffer.
|
@yunyul can you please take a look? |
yunyu commentedMar 26, 2017
•
edited
Several users in the IRC (@ivanovdns, @alexlehm) reported that all vertx-template-engines tests with newlines were failing on Windows.
This is because Git on Windows checks out as CRLF by default, and the template engines in question just use the provided line endings. The fact that Windows line endings are ever returned is an issue in the implementation, as usage of '\n' is extremely prevalent throughout the Vert.x codebase, including the tests. We can't just use System.getProperty("line.separator") in the tests, as the behavior isn't reliable and some developers may disabled autocrlf.
These commits fix this behavior by introducing a performant CRLF-to-LF converter adapted from Netty's ByteBufInputStream in Utils#readFileToString, and has the side effect of fixing all the template tests on Windows. For the latter reason, no additional tests should be needed.
If the behavior shouldn't be changed, a form of line ending detection should be done in the tests.