Skip to content

Over-indent multiline interpolations - #13

Closed
dralletje wants to merge 7 commits into
dmnd:mainfrom
dralletje:master
Closed

dralletje wants to merge 7 commits into
dmnd:mainfrom
dralletje:master

Conversation

@dralletje

@dralletje dralletje commented Sep 19, 2017 •

Copy link
Copy Markdown

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 🙃

Comment thread __tests__/dedent-tests.js
fourth
`
).toMatchSnapshot();
});

@rattrayalex-stripe rattrayalex-stripe Sep 19, 2017 •

Copy link
Copy Markdown

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
Copy Markdown
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

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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
Copy Markdown
Author

Added another commit to make a bit more strict

@rattrayalex-stripe

Copy link
Copy Markdown

cc @dmnd is this mergeable?

@ZhouHansen

Copy link
Copy Markdown

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

@JoshuaKGoldberg

Copy link
Copy Markdown
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
Copy Markdown
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