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

support for oxford comma. #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jhullfly
Copy link

@jhullfly jhullfly commented Feb 21, 2019

Support for the oxford comma.
arrayToSentence(['A', 'B'], {oxfordComma: true}) //=> 'A and B'
arrayToSentence(['A', 'B', 'C'], {oxfordComma: true}) //=> 'A, B, and C'

@coveralls

This comment has been minimized.

@coveralls

This comment has been minimized.

Copy link
Owner

@shinnn shinnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No description provided.

Describe the intent of this patch. I thought of this feature when I started creating this library, but soon noticed it's just an alias of lastSeparator: ', and ':

arrayToSentence(['A', 'B', 'C'], {lastSeprator: ', and '}) //=> 'A, B, and C'

Of course I'll consider merging this PR if you can convince me this option is not just an alias, though.

.gitignore Outdated
@@ -1,3 +1,5 @@
.nyc_output
coverage
node_modules
.idea
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. Add this to your global gitignore instead.

@@ -65,6 +65,20 @@ arrayToSentence(['Earth', 'Wind', 'Fire'], {
}); //=> 'Earth, Wind & Fire'
```

### options.oxfordComma
Copy link
Owner

@shinnn shinnn Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option name is confusing when you use this library for non-English languages; separator could not be a comma in those cases.

arrayToSentence(['A', 'B', 'C'], {
oxfordComma: true
}); //=> 'A, B, and C'

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

.gitignore Outdated
@@ -1,3 +1,5 @@
.nyc_output
coverage
node_modules
.idea

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@jhullfly
Copy link
Author

Unfortunately

arrayToSentence(['A', 'B'], {lastSeparator: ', and '}) //=> 'A, and B'

but

arrayToSentence(['A', 'B'], {oxfordComma: true}) //=> 'A and B'

So it is not just an alias (I added a test to reflect this)

I agree the name is odd for non-english. However, it is not clear that this feature makes sense outside of english. This is also called the serial comma https://en.wikipedia.org/wiki/Serial_comma although I don't think that term is as widely known not does it seem any more suitable.

Open to other suggestions for the name of the feature.

Jesse

@jhullfly
Copy link
Author

This project https://www.npmjs.com/package/humanize-duration calls it serialComma

@shinnn shinnn self-requested a review April 17, 2019 20:57
@shinnn
Copy link
Owner

shinnn commented May 1, 2019

arrayToSentence(['A', 'B'], {lastSeparator: ', and '}) //=> 'A, and B'

but

arrayToSentence(['A', 'B'], {oxfordComma: true}) //=> 'A and B'

So it is not just an alias (I added a test to reflect this)

Ahh, I didn't notice it when I created this module! Thanks for the clarification.

Copy link
Owner

@shinnn shinnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of oxfordComma option, I'd like to propose a more generic one pairSeparator that will only be applied to two-element arrays.

If a user want to use Oxford commas, the code would look like:

const options = {
  lastSeparator: ', and ',
  pairSeparator: ' and '
};

arrayToSentence(['A', 'B', 'C']); //=> 'A, B, and C'
arrayToSentence(['A', 'B']); //=> 'A and B'

What do you think, @jhullfly ?

P.S. Thanks for your patience over a month!

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.

None yet

3 participants