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

Adjacent strings should never be... adjacent to each other #684

Closed
srawlins opened this issue Apr 25, 2018 · 7 comments
Closed

Adjacent strings should never be... adjacent to each other #684

srawlins opened this issue Apr 25, 2018 · 7 comments

Comments

@srawlins
Copy link
Member

srawlins commented Apr 25, 2018

Hopefully not alone on this one:

var a = 'Multiline strings keep leading space\n' 'I write adjacent strings.\n';

I am using adjacent strings here, instead of a multiline string, because of leading whitespace. So I want the strings on separate lines. I'd expect this to format as:

var a = 'Multiline strings keep leading space\n'
    'I write adjacent strings.\n';

But it doesn't. I think 99% of adjacent string usage is where multiline strings would work, but the leading whitespace retention makes it unattractive.

The only use case I can think of for adjacent strings on the same line is mixing raw and not raw (or mixing double and single quotes):

var a = '$var' r'$money' 'newline\n' r'regexp \s*\d+\Z';

Is that a thing (not that it's super readable either)? Anyways, I can't see a way to force newlines between my two strings (no trailing comma trick here). Even comments in between don't force it ☹️

var b = 'text' /* comment */ 'more text' /* comment */;
@munificent
Copy link
Member

Hmm, yeah, I'm guessing most uses of adjacent strings are intended to be on subsequent lines, though it would be good to verify that.

I'm worried about how much churn this change might make, but it might be doable and worth doing.

@a14n
Copy link
Contributor

a14n commented Apr 25, 2018

What about wrapping when a string ends with \n?

@srawlins
Copy link
Member Author

Bob, I'm assuming this is a super tiny change. If you can paste a patch here, I'd be happy to run it over (all) internal code and see what comes up.

@munificent
Copy link
Member

@a14n, that's a really interesting idea. Definitely worth investigating.

Bob, I'm assuming this is a super tiny change.

I think so, but it may be a little tricky getting things like the indentation correct. Adjacent strings have been a difficult area of the formatter to work on because it's often not clear what kind of indentation is desired in what contexts or how to ensure that's what users get.

@srawlins
Copy link
Member Author

I see. Not high priority, but I'd be happy to do an analysis if you get to it.

@nshahan
Copy link

nshahan commented Jun 22, 2023

There is a pattern in DDC that tickles this issue every once and a while when I'm missing a comma between two adjacent string arguments and they accidentally become adjacent strings on the same line. I wish they would get formatted to separate lines because then I feel like the missing comma would be easier to notice.

Also, there is a violation of the reversible property of the formatter here that I think would go away if adjacent strings were always forced to separate lines.

@munificent
Copy link
Member

Your wish is granted (admittedly after a long delay). The forthcoming tall style always forces newlines between adjacent strings. In other words, it has the principle that the only reason to use adjacent strings is to split them across lines. With the new formatter, you get:

var a =
    'Multiline strings keep leading space\n'
    'I write adjacent strings.\n';

var a =
    '$bar'
    r'$money'
    'newline\n'
    r'regexp \s*\d+\Z';

var b =
    'text' /* comment */
    'more text' /* comment */;

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

No branches or pull requests

4 participants