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(markdown): add `proseWrap: "preserve"` option #3340

Merged
merged 12 commits into from Dec 1, 2017

Conversation

Projects
None yet
6 participants
@ikatyang
Member

ikatyang commented Nov 28, 2017

Fixes #3302

Since detecting sentences in different natural languages is hard, preserving original linebreaks should be a good alternative.

Ref: #3302 (comment)

@azz

This comment has been minimized.

Member

azz commented Nov 28, 2017

Maybe this should be the default? Currently proseWrap: true causes issues in BitBucket server because its version of Markdown is much more sensitive to line breaks.

@ikatyang

This comment has been minimized.

Member

ikatyang commented Nov 28, 2017

It will be an actual breaking change if we change the default, though the change is to preserve linebreaks, not sure if we should do so.

@azz

This comment has been minimized.

Member

azz commented Nov 29, 2017

@vjeux Thoughts on making this the default?

@ikatyang ikatyang added this to the 1.9 milestone Nov 29, 2017

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Nov 29, 2017

I am not a fan of the current default, breaking words to 80 columns for markdown is imo a bad idea. Honestly I think that the best default would be to put everything in one line, but making preserve would be a step in the right direction from my point of view.

ikatyang added some commits Nov 29, 2017

@azz

azz approved these changes Nov 29, 2017

Might want to update playground as well?

@ikatyang

This comment has been minimized.

Member

ikatyang commented Nov 29, 2017

The playground option should be in another PR, tracked in #3338.

@SimenB

This comment has been minimized.

Contributor

SimenB commented Nov 29, 2017

So this basically means prettier won't touch paragraphs all all? If so I'd prefer the default being all in a single line - I expect prettier to normalize stuff not just ignore inconsistencies.

I do think this option is a good alternative, but I do think the value proposition of either of the two existing options are higher

@ikatyang

This comment has been minimized.

Member

ikatyang commented Nov 29, 2017

I originally choose "always" by default as it looks the most "prettier"-style, wrapping everything fits the printWidth, but if we're talking about the correctness, "preserve" should be the best since the markdown renderer used by GitHub comment/BitBucket are linebreak-sensitive, or if we're talking about which one is the most useful, it should be "never".

I don't have strong preference here.

@suchipi

This comment has been minimized.

Member

suchipi commented Nov 29, 2017

My vote goes towards making "preserve" the default.

@azz

This comment has been minimized.

Member

azz commented Nov 29, 2017

@SimenB In principle I agree, but unfortunately (as @ikatyang mentioned) there are multiple variants of Markdown that are all a little different when it comes to line breaks, so, to me, favouring not breaking the rendering of existing Markdown makes sense for the default value.

@ikatyang ikatyang referenced this pull request Nov 29, 2017

Merged

chore(playground): add new options #3350

1 of 1 task complete

ikatyang and others added some commits Nov 30, 2017

if (typeof normalized.proseWrap === "boolean") {
normalized.proseWrap = normalized.proseWrap ? "always" : "never";
console.warn(

This comment has been minimized.

@azz

azz Nov 30, 2017

Member

Do we still use deprecated.js? It seems to be outdated.

This comment has been minimized.

@ikatyang

ikatyang Nov 30, 2017

Member

It seems it's only used for useFlowParser warning.

I think we can refactor those option warnings/redirections using #3352.

@azz

This comment has been minimized.

Member

azz commented Dec 1, 2017

LGTM.

@azz azz merged commit 073c0b1 into prettier:master Dec 1, 2017

4 checks passed

codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 96.96% (+<.01%) compared to f119d4a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ikatyang ikatyang deleted the ikatyang:feat/markdown-preserve-prose-wrap branch Dec 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment