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.

Sign up for GitHub

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

Jump to bottom

Add newline option, resolves #5 #6

Closed
wants to merge 3 commits into from
Closed

Add newline option, resolves #5 #6

wants to merge 3 commits into from

Conversation

ThomDietrich
Copy link

@ThomDietrich ThomDietrich commented Jul 31, 2017 •
edited
Loading

Resolves #5

Feel free to adapt as you see fit.

@wooorm wooorm mentioned this pull request Jul 31, 2017
Closed
@wooorm
Copy link
Member

wooorm commented Jul 31, 2017

Looks cool! How about, accepting something an object with the following properties.

  • type: 'space' or 'newline' (default 'space')
  • count: number (default 1)

If a string is passed in instead, treat it as type, and if a number is passed in, treat it as count?

That way the current system keeps working, plus we add a lot more extensibility!

@ThomDietrich
Copy link
Author

ThomDietrich commented Aug 1, 2017 •
edited
Loading

I'm not sure about the benefits. The current solutions allows for three distinct values of the preferred option. The new solution would not change that but the number option is irrelevant to the newline type - seems worse to me!?

Btw. there is currently no value checking of the preferred variable. Shouldn't there be an error if the user configured preferred: 'bolux123'

@wooorm
Copy link
Member

wooorm commented Aug 1, 2017

The current system allows a number to be passed in, defaulting (null, undefined, 0, etc) to 1.
My thinking here was that it would be useful to allow people to use multiple newlines. But I don’t really think that’s very useful.

Would it be reasonable to expect people to want to use \t, or \r\n, and should we support that?

@wooorm
Copy link
Member

wooorm commented Aug 12, 2017

I worked on this a bit and I feel the 'newline' option is worded a bit weird: what it actually does is check that no spaces are used! Which you could fix by adding a newline, but a tab or something else also fixes this!

How about 0 instead? Or 'none'?

@wooorm
Copy link
Member

wooorm commented Aug 17, 2017

@ThomDietrich Friendly ping!

@ThomDietrich
Copy link
Author

Hey @wooorm, deeply sorry for beeing the bad conversation side for once ^^ I'm currently on vacation and will have another look at this next week, including adding my thoughts to your questions. Best! ;)

@wooorm
Copy link
Member

wooorm commented Aug 22, 2017

Have fun on your holiday! No worries!

@wooorm
Copy link
Member

wooorm commented Sep 4, 2017

@ThomDietrich Friendly ping!

@ThomDietrich
Copy link
Author

ThomDietrich commented Sep 4, 2017 •
edited
Loading

I've not forgotten about this :( sorry for the long delay and thanks for keeping up the spirit.

Regarding your comments: The goal of my addition is to check for at least one line break between sentences. The reason for that is, so that markdown paragraphs can be managed with git and GitHub in a meaningful way (see here for a good discussion with pros and cons). In our documentation projects we decided to go with the "newline" rule in the style guide and it turned out to be quite helpful.

The rule should hence identify everything that is not "sentence newline* sentence".
Now that I did check the code again, you are of course right. I've not covered all cases and should work on that. Still we should agree on the configuration option. I'm seeing three meaningful options:

  • 1 space between sentences
  • 2 spaces between sentences
  • one line break between sentences

All three options should ignore multiple line breaks, as that is a valid markdown element.

How do we want to deal with tabs? Imho they should be treated as an error.

@ThomDietrich ThomDietrich mentioned this pull request Jan 13, 2018
Merged
@ThomDietrich
Copy link
Author

@wooorm wdyt?

@wooorm wooorm mentioned this pull request Jan 8, 2019
Merged
@wooorm wooorm closed this in #7 Jan 8, 2019
wooorm added a commit that referenced this pull request Jan 8, 2019
@wooorm
Add support for newline option
a568b45 
Previously, spaces between sentences where checked, and warned about if they
did not match the given preferred size.  Now, some people like newlines between
sentences.  Which makes sense.  But the previous code did not allow for that.
These changes, based on GH-5 and GH-6, adds support for checking that spaces
are *not* used between sentences.

Additionally, errors are thrown for out-of-range values (lower than 0 or higher
than 2), and non-number values.

Note however that this plugin only checks white-space consisting of just spaces
between sentences.  It doesn’t check other delimiters (such as tabs, newlines,
or mixed white-space).

Closes GH-5.
Closes GH-6.

Co-authored-by: Thomas Dietrich <[email protected]>
@wooorm
Copy link
Member

wooorm commented Jan 8, 2019

I’m so sorry this slipped through the cracks for one and a half year 😢
I reworked your changes a bit, I think the changes I made just now make sense.
Thanks for working on this previously!

@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Aug 13, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Newline as an option
2 participants
@ThomDietrich @wooorm