Skip to content

Add option to enforce certain line endings#5327

Merged
ikatyang merged 26 commits intoprettier:masterfrom
kachkaev:eol-option
Nov 6, 2018
Merged

Add option to enforce certain line endings#5327
ikatyang merged 26 commits intoprettier:masterfrom
kachkaev:eol-option

Conversation

@kachkaev
Copy link
Copy Markdown
Member

@kachkaev kachkaev commented Nov 2, 2018 •
edited
Loading

This PR contains a minimal viable implementation of #5320.

There is also a sidecar PR in another repo to read .editorconfig option as suggested by @evilebottnawi. I believe it is ready to be reviewed and can be merged after we close the first item in the TODO list below. We need to release a new version of editorconfig-to-prettier before completing this feature.

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • I’ve read the contributing guidelines.

TODO (based on reviews)

  • Agree on the option name:

    • eol (originally proposed) – a commonly known abbreviation, but may be confusing for newcomers
    • ✅ endOfLine / --end-of-line (suggested by two maintainers) – easier for newcomers, but takes longer to write; similar to .editorconfig's end_of_line
    • lineEnding(s?) / --line-ending(s?) – a commonly used term in human-to-human communication

  • Decide if we want to support --endOfLine=cr at all. I'm not sure if Prettier can successfully parse files without LF at the moment. In any case, only CRLF and LF are prevalent (I haven’t seen anyone using CR only). Discussion: 4th conversation in Add option to enforce certain line endings #5327 (review) (marked as resolved by @ikatyang). Solution: keep cr amongst values for consistency with what .editorconfig supports.

  • Merge [WIP] Convert .editorconfig’s end_of_line option to Prettier’s eol option josephfrazier/editorconfig-to-prettier#4 and replace a temporary used fork with the original package.

Looking forward for your thoughts folks 🙌

Future work

  • Discuss if it's possible to apply an explicitly defined eol for files that have a syntax error don't have a known grammar (e.g. .gitignore). This should make it possible to prettier --write '**/*' or prettier-check '**/*'. We can do this in follow-up issues and PRs.

@kachkaev kachkaev mentioned this pull request Nov 2, 2018
Closed
@j-f1 j-f1 added the status:wip Pull requests that are a work in progress and should not be merged label Nov 2, 2018
@j-f1 j-f1 changed the title [WIP] EOL option EOL option Nov 2, 2018
@kachkaev kachkaev mentioned this pull request Nov 2, 2018
Merged
@kachkaev kachkaev changed the title EOL option [RFC] Add option to enforce certain line endings Nov 4, 2018
@j-f1 j-f1 removed the status:wip Pull requests that are a work in progress and should not be merged label Nov 4, 2018
lipis
lipis reviewed Nov 4, 2018
View reviewed changes
@lipis
Copy link
Copy Markdown
Member

lipis commented Nov 4, 2018

So this is ready to be merged? Maybe we should update the TODO list then..

@kachkaev
Merge commit 'e17512adcd21c23b3bbf3c8308c2f988ec89fb56' into eol-option
1a5c747 
# Conflicts:
#	tests_integration/__tests__/__snapshots__/support-info.js.snap
@lipis
Copy link
Copy Markdown
Member

lipis commented Nov 4, 2018

Also if this is part of the 1.15 milestone we will have to update the blog post as well!

@lipis lipis mentioned this pull request Nov 4, 2018
Closed
11 tasks
@kachkaev
Copy link
Copy Markdown
Member Author

kachkaev commented Nov 4, 2018 •
edited
Loading

@lipis the PR is RFC (Ready for comments). I'm waiting for feedback from folks participating in #5320 to update TODOs. To be honest, I'd love --eol to become a part of 1.15, because consistently dealing with EOLs is a big pain for my team 🙏

@lipis lipis mentioned this pull request Nov 4, 2018
Merged
@kachkaev
Copy link
Copy Markdown
Member Author

kachkaev commented Nov 5, 2018

Looks like the only thing left to do is to wait for the new version of editorconfig-to-prettier following the merge of josephfrazier/editorconfig-to-prettier#4! 🚀

josephfrazier pushed a commit to josephfrazier/editorconfig-to-prettier that referenced this pull request Nov 5, 2018
@kachkaev @josephfrazier
Convert EditorConfig's end_of_line option to Prettier’s endOfLine opt…
402e318 
…ion (#4)

Partially addresses prettier/prettier#5320. Helps implement prettier/prettier#5327.
@josephfrazier
Copy link
Copy Markdown
Collaborator

josephfrazier commented Nov 5, 2018

Looks like the only thing left to do is to wait for the new version of editorconfig-to-prettier following the merge of josephfrazier/editorconfig-to-prettier#4! 🚀

Done, you can upgrade to version 0.1.0 now!

@kachkaev kachkaev changed the title [RFC] Add option to enforce certain line endings Add option to enforce certain line endings Nov 5, 2018
@kachkaev
Copy link
Copy Markdown
Member Author

kachkaev commented Nov 5, 2018

Many thanks for doing this so quickly @josephfrazier! 💯

This PR should be ready for a final review and can be merged after that! @lipis can we squeeze it into 1.15 given that all is done? 😅

@duailibe
Copy link
Copy Markdown
Collaborator

duailibe commented Nov 5, 2018

@ikatyang You're pretty much managing the 1.15 release, I think you can decide to include in this release or not.

@ikatyang
Copy link
Copy Markdown
Member

ikatyang commented Nov 5, 2018

I'd like to wait for a day to see if there's any objection, it will be included in 1.15 if everything goes well.

lydell
lydell reviewed Nov 5, 2018
View reviewed changes
@kachkaev
Copy link
Copy Markdown
Member Author

kachkaev commented Nov 5, 2018 •
edited
Loading

After some more thinking, I changed the order of option values both in docs and cli (auto|cr|crlf|lf → auto|lf|crlf|cr). It used to be alphabetic and now the values go in the order of prevalence. See a resolved conversation in @lydell's review for details.

@ikatyang if you don't agree with this change, simply chop off my last commit before merging. Let me know if you need help with anything else!

@kachkaev
Copy link
Copy Markdown
Member Author

kachkaev commented Nov 6, 2018

Any final comments or suggestions folks? If not, shall we...

? 😅

@ikatyang
Copy link
Copy Markdown
Member

ikatyang commented Nov 6, 2018

Don't worry, I'll merge it later. I was fixing regressions.

@ikatyang ikatyang modified the milestones: 1.16, 1.15 Nov 6, 2018
@ikatyang ikatyang merged commit b87fe4c into prettier:master Nov 6, 2018
@ikatyang
Copy link
Copy Markdown
Member

ikatyang commented Nov 6, 2018

Thanks!

ikatyang added a commit to ikatyang/prettier that referenced this pull request Nov 6, 2018
@ikatyang
docs: add description for prettier#5327
444e9ea
@kachkaev
Copy link
Copy Markdown
Member Author

kachkaev commented Nov 6, 2018

Amazing!!! Many thanks for all your prompt comments and very useful reviews! I'm soooooo glad we could squeeze this feature into 1.15 that I'm jumping through the roof! 😅

@damccorm damccorm mentioned this pull request Nov 8, 2018
Merged
2 tasks
@patricklafrance patricklafrance mentioned this pull request Nov 15, 2018
Closed
@kachkaev kachkaev deleted the eol-option branch December 15, 2018 22:31
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Mar 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lipis lipis lipis left review comments

@lydell lydell lydell left review comments

@suchipi suchipi suchipi approved these changes

@ikatyang ikatyang ikatyang approved these changes

@j-f1 j-f1 j-f1 approved these changes

@duailibe duailibe Awaiting requested review from duailibe

@vjeux vjeux Awaiting requested review from vjeux

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

1.15

Development

Successfully merging this pull request may close these issues.

9 participants

@kachkaev @lipis @suchipi @ikatyang @josephfrazier @duailibe @vjeux @lydell @j-f1