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

feat: dedent spacing #147

Closed
wants to merge 1 commit into from
Closed

Conversation

bennypowers
Copy link

Closes #146

@syavorsky
Copy link
Owner

@bennypowers thanks for the PR, but I want to circle back to what I was suggesting in #146 comments

The spacing option was intentionally kept limited to avoid complexity.

I don't agree with allowing spacing: 'dedent'. That option got to stay simple as is. For all other transformations, I suggest applying them as in this example without touching top-level API. Indentation is not a parsing option but rather a transformation you apply to the tokenized source. So for the dedent it would look like

const { dedent } = transforms;
const parsed = parse(source);
const stringified = stringify(dedent(parsed[0]));

all config permutations handled in the main flow were a huge pain in 0.x version. On other hand, using flow() allows customizing the flow with interference

if you still willing to land this contribution, can we shape it after transforms/align.ts which is a pretty similar transformation? So it will boil down to

transforms/dedent.ts
tests/unit/transforms-align.spec.ts
tests/e2e/transforms.spec.js

@bennypowers
Copy link
Author

doing so would require reimplementing dedent to take markers and return a Transform, when it's already implemented as string => string. What do you think?

@syavorsky
Copy link
Owner

Yes, dedent would have to be a function returning configured Transform. It's the already existing flow for align and indent, there is no good reason dedent should be one-off

@bennypowers
Copy link
Author

Unfortunately I don't have the time right now to rewrite an existing package to suit the custom tokenizer that comment-parser uses. I think it would be a great feature but for now I'll have to apply workarounds like dedenting the output in my consuming code.

Thanks for considering this PR

@bennypowers bennypowers closed this Dec 8, 2021
@syavorsky
Copy link
Owner

@bennypowers no prob. Thanks for the feature suggestion, I will try to land it myself sometime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: { spacing: 'dedent' }
2 participants