Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

Map insert_final_newline to insertFinalNewline #8

Merged
merged 4 commits into from
Aug 25, 2020

Conversation

dcyriller
Copy link
Contributor

Description

This PR proposes to read editorconfig's insert_final_newline when it is set to false.

Motivation

I know this PR might sound odd. Because Prettier follows unix guideline regarding the final newline (Prettier doc reference).

Yet, handlebars / glimmer does not follow this standard (see reference here) because it would insert empty nodes at the bottom of the component's file.

So for now Prettier respects this handlebars choice and remove the final newline. But this is somehow inconsistent with other languages and it creates discussion / lack of understanding etc. One possibility could be to respect instead the user choice made with .editorconfig's insert_final_newline option. Hence this PR.

In a follow-up, we'd use that insertFinalNewline option in handlebars printer to insert or remove the final newline.

Links

Copy link
Owner

@josephfrazier josephfrazier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough details on why this is desired! Do you see any reason not to support parsing the "true" value as well? I added a couple of commits to do that for completeness.

test.js Outdated
assert.deepStrictEqual(
editorconfigToPrettier({ insert_final_newline: "false" }),
{}
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need this test at all? Not sure how realistic this scenario is...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to test an invalid value (wrong type here). But it might be well unnecessary. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit to remove the unecessary test scenarios.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I think that since this function expects an already-parsed editorconfig object, it's safe to assume that the values will be valid, so I don't want to worry about testing invalid values. Thanks for removing the extra tests.

@dcyriller
Copy link
Contributor Author

Do you see any reason not to support parsing the "true" value as well? I added a couple of commits to do that for completeness.

I did that to be the less verbose possible. But suport parsing the "true" value as well seems good to me 👍

@josephfrazier
Copy link
Owner

Makes sense to me, I'll go ahead and merge this, and likely release a new version as well. Will keep you posted 👍

@josephfrazier josephfrazier changed the title Read insert_final_newline when set to false Map insert_final_newline to insertFinalNewline Aug 25, 2020
@josephfrazier josephfrazier merged commit e2ff11a into josephfrazier:master Aug 25, 2020
@josephfrazier
Copy link
Owner

Just published v0.2.0, thanks again for the contribution! https://www.npmjs.com/package/editorconfig-to-prettier/v/0.2.0

@dcyriller dcyriller deleted the insert-final-newline branch August 25, 2020 12:04
@dcyriller
Copy link
Contributor Author

awesome @josephfrazier thank you!

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

Successfully merging this pull request may close these issues.

2 participants