-
Notifications
You must be signed in to change notification settings - Fork 36
Over-indent multiline interpolations #13
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
Conversation
fourth | ||
` | ||
).toMatchSnapshot(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding a test like the one referenced in #12 , namely:
const weird = dedent`
foo.
${[1,2,3].map(x => dedent`
hello
world
`).join('\n')}
bar.
`;
which should produce (as it does):
foo.
hello
world
hello
world
hello
world
bar.
Possibly also
const weirder = dedent`
foo.
${[1,2,3].map(x => dedent`
hello
world
`).join('\n')}
bar.
`;
which I think should produce
foo.
hello
world
hello
world
hello
world
bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel these extra tests are not necessary, as this is covered by the 'non-map' version of the tests... dunno how the author feels about tests testing the same thing :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... Yes, you're correct; I wasn't reading very carefully. Thanks!
The inner template should probably include indents/dedents, though, eg;
dd`
first
${dd`
second
third
fourth
`}
fifth
`
to ensure that "inner template" logic is the same as "outer template" logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(to be clear, I think the above is purely optional and I would expect it to pass today)
Added another commit to make a bit more strict |
cc @dmnd is this mergeable? |
Hi! @rattrayalex-stripe you can use endent to achieve what you want before this pr merged. |
Note: I posted a question in #12 about whether we want this to always be the case, or an opt-in. |
Closing for now to keep the queue small - let's discuss in #12. Cheers! |
I made this PR after needing the same features as #12
It looks at the value in interpolations and adds the indentation to every but the first line (because that one already has the indentation).
I'm not sure if you'd want this in the package, as it assumes a lot more stuff, but on the other hand I can not imagine a scenario where you don't want this 🙃