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

Fix widont newline behavior. #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryneeverett
Copy link

Fix #38.

Widont shouldn't count newlines as spaces. This is achieved with a regex inspired by this SO post. Rather than checking for a space (which includes newlines), we check for both (a) not not a space, and (b) not a newline.

@chrisdrackett
Copy link
Collaborator

Thanks for the fix! I don't have time to check this right away, but I'll try and get it tested and merged this week!

@Kristinita
Copy link

Kristinita commented Apr 10, 2017

@mintchaos , @chrisdrackett , @justinmayer , please, review this pull request.

Thanks.

@justinmayer
Copy link
Collaborator

@ryneeverett: Please accept my sincere apologies for the absurdly long delay in reviewing your contribution. Truly.

In my cursory review of the changed output, it does indeed seem improved when there is a newline in-between the last two words. I found a case, however, in which the presence of two spaces were previously replaced with a  , but the new regex in this PR leaves those two spaces in place. For example:

“Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.”  —Socrates

Following is how Typogrify treats the two spaces between the closing quotation mark and the em-dash above:

  • Current master: two spaces are replaced with &nbsp
  • This PR: two spaces are left untouched

That leaves me with two questions:

  1. Was this change intentional? I get the feeling that it isn't, given what the stated objective is.
  2. Is this change beneficial? I imagine not, but what do other folks think?

@ryneeverett
Copy link
Author

I don't have much more insight into what I was thinking six years ago than you do. :) If that's indeed the effect then I'd agree this change is not beneficial. Feel free to close.

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.

widont line break/newline behavior
4 participants