-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add an option to recursively dedent nested template literal strings #12
Comments
I needed exactly the same today, so #13 was born. What would your preferred behaviour for the last example be?
|
Awesome! Will discuss expected behavior in the PR |
indentjs/endent endows suitable indentation for multiline interpolation. |
@ZhouHansen This issue exist also with endent: https://runkit.com/embed/fx2ssy6zqm9x |
@bertho-zero , here's the right usage: https://runkit.com/zhouhansen/5b023b1fd78bc100121e15f8 |
This is not a neested endent. |
@bertho-zero You are right. This is not because of the multiple indentation though. It is a bug in endent that makes it not work well with string that start without indentation at all. This one (that clearly has one line indented and the other not indented) will output a totally not indented string Once you add a layer of indentation to all your lines, it gives the right output @ZhouHansen This is a bug in endent - ofcourse you want to first apply endent to the substring.. not sure why you made your "right usage" so wrong ;) |
@dralletje that will be very helpful! |
@dralletje you didn't compile your fork, so it still has the same dist code. |
@d07RiV Good find, updated it :) |
Thank you @ZhouHansen , I confirm that the endent package has the behavior I want – I have replaced all uses of By the way, it looks like |
Hmm, I can see why you'd want this behavior... but also it's a bit counter-intuitive to how the package is positioned right now IMO. Today, it's used as a "dedent everything in this string" utility - not a "dedent this string, and still allow indents in some contents". Perhaps this can become an opt-in? A couple of API starting ideas.. dedent.recursive`
${text}
`;
dedent({ recursive: true })`
${text}
`; Thoughts, everyone? |
This is fixed with |
@JoshuaKGoldberg thanks for pointing me here! To provide some feedback on this - I use this package a lot (I write a lot of cli tooling in particular that spits out formatted error messages to users) and 10/10 times this is the default behavior i'm looking for. That said, I understand and totally respect it's a question of scope and not wanting to fundamentally change (in a backwards incompatible way) the behaviour of this significantly depended-on library! With that in mind, I feel like it's worthy enough of being a top level namespace thing (rather than buried as an option), and so I like what you initially suggested the most:
fwiw in terms of API ideas, here's some prior art for a similar thing in execa: https://github.com/sindresorhus/execa#shared-options - so you'd suggest something like:
I don't personally love this, the 3rd option (which could go alongside as an alias of the first option)
|
The For now, let's add it in as an opt-in option: |
Heh, hey @JoshuaKGoldberg ! Good to see you here! Sorry I missed this before. FWIW, it's not my expected behavior to have the library dedent most things, except child strings. I consider the current behavior a bug, not a separate feature, though I may be missing something. I wouldn't want to have to use an option for a library like this – more specifically, I wouldn't want to have to police that everyone in my codebase always calls the recursive version. I could maybe be okay with For now, personally, I'd probably simply stick with Just my 2c! |
Hijacked by @JoshuaKGoldberg on September 4th, 2023: see #12 (comment). Now accepting PRs for a
dedent({ recursive: true })
option.results in
instead of
This is unfortunate because it would be nice to compose dedented strings.
If you decide to fix this, any test cases should also check that inlined dedents make sense:
The text was updated successfully, but these errors were encountered: