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

Add option to enforce certain line endings #5327

Merged
merged 26 commits into from Nov 6, 2018

Conversation

@kachkaev
Copy link
Contributor

commented Nov 2, 2018

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 #5327 (review) (marked as resolved by @ikatyang). Solution: keep cr amongst values for consistency with what .editorconfig supports.

  • Merge 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 added some commits Nov 2, 2018

@kachkaev kachkaev force-pushed the kachkaev:eol-option branch from 6def83c to 4b783ef Nov 4, 2018

kachkaev added some commits Nov 4, 2018

@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 label Nov 4, 2018

Show resolved Hide resolved package.json Outdated
@lipis

This comment has been minimized.

Copy link
Member

commented Nov 4, 2018

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

Merge commit 'e17512adcd21c23b3bbf3c8308c2f988ec89fb56' into eol-option
# Conflicts:
#	tests_integration/__tests__/__snapshots__/support-info.js.snap
@lipis

This comment has been minimized.

Copy link
Member

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 referenced this pull request Nov 4, 2018

Closed

1.15 Release #5297

11 of 11 tasks complete
@kachkaev

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2018

@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 referenced this pull request Nov 4, 2018

Merged

docs(blog): 1.15 release #5296

@lipis lipis requested review from duailibe, ikatyang, j-f1 and lydell Nov 4, 2018

josephfrazier added a commit to josephfrazier/editorconfig-to-prettier that referenced this pull request Nov 5, 2018

@josephfrazier

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

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? 😅

@j-f1

j-f1 approved these changes Nov 5, 2018

@duailibe

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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.

Show resolved Hide resolved docs/options.md Outdated
@kachkaev

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

After some more thinking, I changed the order of option values both in docs and cli (auto|cr|crlf|lfauto|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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

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

? 😅

@ikatyang

This comment has been minimized.

Copy link
Member

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

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 97.02% (+0.01%) compared to 2136fdc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@ikatyang

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Thanks!

ikatyang added a commit to ikatyang/prettier that referenced this pull request Nov 6, 2018

@kachkaev

This comment has been minimized.

Copy link
Contributor Author

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! 😅

@kachkaev kachkaev deleted the kachkaev:eol-option branch Dec 15, 2018

@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.
You can’t perform that action at this time.