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 print of block string with leading space and quotation #1190

Merged
merged 1 commit into from
Jan 9, 2018

Conversation

IvanGoncharov
Copy link
Member

When I looked at #1188 I noticed that current implementation doesn't handle strings like that:

"""    space-led value "quoted string"
"""

Currently it's printed as:

"""    space-led value "quoted string""""
                                     ^ breaks parser

@@ -249,7 +249,7 @@ function wrap(start, maybeString, end) {
}

function indent(maybeString) {
return maybeString && maybeString.replace(/\n/g, '\n ');
return maybeString && ' ' + maybeString.replace(/\n/g, '\n ');
Copy link
Member Author

@IvanGoncharov IvanGoncharov Jan 2, 2018

Choose a reason for hiding this comment

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

Can be replaced with:

return maybeString && maybeString.replace(/^|\n/g, '$&  ');

But I chose easier to read version even those it's more verbose.

@leebyron
Copy link
Contributor

leebyron commented Jan 8, 2018

What do you think about just simplifying block string printing to always be:

"""
Content
"""

That would require updating some test cases, but would be much more predictable

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Jan 8, 2018

What do you think about just simplifying block string printing to always be:

@leebyron I'm confused 😕
If you transform """ space-led""" into:

""""
   space-led
"""

You would lose prefix spaces.
Or do you want to change the specification to remove leading and trailing spaces from block string?

@leebyron
Copy link
Contributor

leebyron commented Jan 8, 2018

Great question about that edge-case. Let me look into that and return to a decision soon

@leebyron
Copy link
Contributor

leebyron commented Jan 9, 2018

Let's stick with this approach, nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants