Skip to content

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

Closed
wants to merge 7 commits into from
Closed

Conversation

dralletje
Copy link

@dralletje dralletje commented Sep 19, 2017

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 🙃

fourth
`
).toMatchSnapshot();
});
Copy link

@rattrayalex-stripe rattrayalex-stripe Sep 19, 2017

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.

Copy link
Author

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

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

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)

@dralletje
Copy link
Author

Added another commit to make a bit more strict

@rattrayalex-stripe
Copy link

cc @dmnd is this mergeable?

@ZhouHansen
Copy link

Hi! @rattrayalex-stripe you can use endent to achieve what you want before this pr merged.

@JoshuaKGoldberg
Copy link
Collaborator

Note: I posted a question in #12 about whether we want this to always be the case, or an opt-in.

@JoshuaKGoldberg JoshuaKGoldberg added the status: in discussion Not yet ready for implementation or a pull request label Jul 2, 2023
@JoshuaKGoldberg
Copy link
Collaborator

Closing for now to keep the queue small - let's discuss in #12. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Not yet ready for implementation or a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants