search

ocsigen / tyxml

Build valid HTML and SVG documents
https://ocsigen.org/tyxml/
Other
166 stars 59 forks source link

Add GitHub Actions workflow #282

Closed smorimoto closed 3 years ago

smorimoto commented 3 years ago

This caused the tests to fail on Windows and needed to be fixed. (In Windows, EOL is treated as \r\n.) https://github.com/ocsigen/tyxml/blob/8551b673857fdc1defebe7c502ffcb920f04009e/implem/tyxml_xml.ml#L95-L100 https://github.com/ocsigen/tyxml/blob/8551b673857fdc1defebe7c502ffcb920f04009e/test/test_html.ml#L42-L72

Drup commented 3 years ago

Thanks! I wanted to do that, but never took the time. Can you confirm that it also tries to install the various packages independently (to make sure tyxml works even without tyxml-jsx, for instance) ?

Apart from that :

  1. Could you put back the long descriptions. I prefer them over the short ones.
  2. The fix for the window test failure is not right (you just fixed the test to avoid showing the issue ...). The right way is to replace the printf \n by %\n so that it outputs plateform-dependent new lines.
Drup commented 3 years ago

(I'm merging in the meantime, since the CI is strictly better with it)

smorimoto commented 3 years ago

Thank you for dealing with that on my behalf!

The fix for the window test failure is not right (you just fixed the test to avoid showing the issue ...). The right way is to replace the printf \n by %\n so that it outputs plateform-dependent new lines.

In this case, I thought it would be better to use \n instead of using the platform-dependent one, isn't it?

Drup commented 3 years ago

In this case, I thought it would be better to use \n instead of using the platform-dependent one, isn't it?

Eh. Unclear. On the other hand, it has been there for the last 10 years and nobody has complained, so ... let's keep it.

smorimoto commented 3 years ago

IIUC, it has long been an issue with OCaml itself that the new line character in the quoted string is CRLF on Windows, and I'm working on fixing it so the tyxml source is pretty much fine. Only the behaviour of quoted strings on Windows is problematic. (So the test should have passed with the quoted string.)