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 invalid writing of special characters when writing to a YAML file #37

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

nipunayf
Copy link
Contributor

@nipunayf nipunayf commented Feb 8, 2024

Purpose

The current implementation of the serializer to switch between plain-style and quoted strings is incomplete for some characters. Hence, the serializer is modified to use the parser to determine if a string can be written in plain-style.

Fixes #6031

Examples

The newline symbol is added in double-quoted scalars to represent new lines in a Ballerina string. For instance, consider the following Ballerina string:

string s = string `first
second
third`;

The above string is represented in YAML as follows:

"first\nsecond\nthird"

Checklist

  • Linked to an issue
  • Updated the changelog
  • Added tests
  • Updated the spec
  • Checked native-image compatibility

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0706d9e) 94.09% compared to head (d2701e3) 94.20%.

Files Patch % Lines
ballerina/modules/parser/parser.bal 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
+ Coverage   94.09%   94.20%   +0.11%     
==========================================
  Files          44       44              
  Lines        2169     2176       +7     
  Branches     1356     1360       +4     
==========================================
+ Hits         2041     2050       +9     
+ Misses        128      126       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

? string `${state.delimiter}${value}${state.delimiter}` : value,
tag
});
if value.includes("\n") {

Choose a reason for hiding this comment

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

Suggested change
if value.includes("\n") {
if value.includes("\n" || value.includes("\r\n") {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting OS-specific new lines is tracked with another issue: ballerina-platform/ballerina-library#4330.

common:Event[] events = check getSerializedEvents(line);
int index = 0;
foreach common:Event event in events {
if event is common:ScalarEvent {

Choose a reason for hiding this comment

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

We need to add a assertFail scenario for the else condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is designed to consider only the ScalarEvents and ignore the rest, as the new lines are only processed in these events. Hence, the events that are not ScalarEvents form a valid stream for the given input.

@NipunaRanasinghe NipunaRanasinghe merged commit 1e1a818 into ballerina-platform:main Feb 12, 2024
6 of 7 checks passed
warunalakshitha pushed a commit to warunalakshitha/module-ballerina-yaml that referenced this pull request Nov 13, 2024
Update ballerina gradle plugin version
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.

Some special characters are not handled when emitting a Ballerina string to a YAML file
3 participants