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

Non-indented lines are ignored when calculating minimum indentation #20

Open
d07RiV opened this issue Jun 1, 2018 · 6 comments
Open
Labels
status: accepting prs Please, send a pull request to resolve this! type: bug Something isn't working :(

Comments

@d07RiV
Copy link

d07RiV commented Jun 1, 2018

Consider this example

dedent`
test
  test2
`

Outputs

test
test2

Regex in line 43 (l.match(/^(\s+)\S+/);) should probably read \s* instead of \s+.

lydell added a commit to lydell/dedent that referenced this issue Sep 6, 2018
ptomato added a commit to Igalia/decimal-playground that referenced this issue Nov 15, 2022
The dedent package seems to be dead. I ran into this issue which has been
open for 4 years and seems pretty serious:
dmnd/dedent#20

Replace the package with string-dedent, an alternative maintained by the
author of the TC39 String.dedent proposal.
@JoshuaKGoldberg JoshuaKGoldberg added type: bug Something isn't working :( status: accepting prs Please, send a pull request to resolve this! status: waiting for author Needs an action taken by the original poster and removed status: accepting prs Please, send a pull request to resolve this! type: bug Something isn't working :( labels May 22, 2023
@JoshuaKGoldberg
Copy link
Collaborator

Hmm, I'm not sure that this is correct actually. Arguably, having a line without any indentation indicates that the string is already dedented, no?

Does anybody have a good use case of why this should be changed?

@JoshuaKGoldberg
Copy link
Collaborator

Thinking more deeply - seems reasonable that many would consider this a bug. Now that we have options (#65) we could take in an option to preserve the old behavior. ignoreNonIndentedLines, maybe?

import dedent from 'dedent';

// test
//   test2
dedent`
test
  test2
`;

// test
// test2
dedent.withOptions({ ignoreNonIndentedLines })`
test
  test2
`;

cc @G-Rath & @Haroenv since you've been active in this repo recently 👋

@JoshuaKGoldberg JoshuaKGoldberg added type: bug Something isn't working :( status: in discussion Not yet ready for implementation or a pull request and removed status: waiting for author Needs an action taken by the original poster labels Jul 30, 2023
@Haroenv
Copy link
Collaborator

Haroenv commented Jul 31, 2023

Makes sense to me, the default behaviour (maybe breaking change again) should be what's described in issue and consider that when something isn't indented, we shouldn't dedent anything (although at that point, I'm not even sure why someone would still use the template string, function could make sense)

@JoshuaKGoldberg JoshuaKGoldberg added status: accepting prs Please, send a pull request to resolve this! and removed status: in discussion Not yet ready for implementation or a pull request labels Sep 5, 2023
@JoshuaKGoldberg
Copy link
Collaborator

Explicitly marking as status: accepting prs to receive a new option for this behavior. 👍

ext added a commit to Forsakringskassan/docs-generator that referenced this issue Nov 5, 2024
@ext
Copy link

ext commented Nov 5, 2024

@JoshuaKGoldberg

I've hit this bug when I use dedent as a function running on potentially already dedented input (user input). To me it seems reasonable that dedent(value) === dedent(dedent(value)) for any given input value.

I could make an attempt to fix this issue, would it be preferable to have a breaking change with a an option to preserve the old behaviour or an option to opt-in for the new behaviour?

EDIT: Personally I believe it should be the default behaviour (breaking)

ext added a commit to Forsakringskassan/docs-generator that referenced this issue Nov 5, 2024
@JoshuaKGoldberg
Copy link
Collaborator

The problem with breaking changes is that dedent is a very old library. A lot of projects have built up behavioral assumptions on how it works now (mandatory xkcd spacebar comic reference). Any small breakage makes it harder for them to upgrade.

Let's consider turning the option on by default as a separate followup from this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepting prs Please, send a pull request to resolve this! type: bug Something isn't working :(
Projects
None yet
Development

No branches or pull requests

4 participants