Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Schema rendering problems with Windows line separators #1129

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

ArminWiebigke
Copy link

@ArminWiebigke ArminWiebigke commented Aug 22, 2024

Fix #1125

@yanns
Copy link
Contributor

yanns commented Aug 26, 2024

Thanks a lot! I very like the test case you added!

The CI checks are failing with a compilation error:

[error] /home/runner/work/sangria/sangria/modules/core/src/main/scala/sangria/renderer/QueryRenderer.scala:480:63: value iterator is not a member of Iterator[String]
[error]       val lines = escapeBlockString(node.value).linesIterator.iterator.map { line =>
[error]     

Do you want to take care of this?

@yanns
Copy link
Contributor

yanns commented Sep 2, 2024

One test seem to fail:

[info]   - should renders comments with various line separators correctly *** FAILED ***
[info]     "..._LF:
[info]     
[info]     __CRLF:
[info]     
[info]     __CR:[

]Empty_lines_above_sh..." did not equal "..._LF:
[info]     
[info]     __CRLF:
[info]     
[info]     __CR:[
[info]     
[info]     __]Empty_lines_above_sh..." (QueryRendererSpec.scala:1018)
[info]     Analysis:
[info]     "..._LF:
[info] 
[info] __CRLF:
[info] 
[info] __CR:[

@ArminWiebigke
Copy link
Author

Seems like \r is not considered a valid line break in Scala 2.12.19 🤔
So I guess we just need to split manually in case any \rs are still contained in the lines returned by linesIterator.

@yanns
Copy link
Contributor

yanns commented Sep 4, 2024

Seems like \r is not considered a valid line break in Scala 2.12.19 🤔 So I guess we just need to split manually in case any \rs are still contained in the lines returned by linesIterator.

Having some code just for scala 2.12 is not optimal.
Why if we would copy / paste the implementation of linesIterator into our code (I mean the implementation that is also considering \r)?

Armin Wiebigke added 3 commits September 4, 2024 20:45
`linesIterator` from Scala is not consistent for our cross-compiled
versions, so we provide an explicit implementation (copied from Scala
2.13.14 StringOps.scala).
Copy link
Contributor

@yanns yanns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@yanns
Copy link
Contributor

yanns commented Sep 4, 2024

I was checking the specs, and your code seems to be compliant with it: https://spec.graphql.org/October2021/#LineTerminator

@yanns yanns added this pull request to the merge queue Sep 4, 2024
Merged via the queue into sangria-graphql:main with commit 0af3c0e Sep 4, 2024
4 checks passed
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.

Schema rendering problems with Windows line separators
2 participants