Skip to content

Replace apostrophe with empty string (like github) #23

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

Merged
merged 1 commit into from
Jun 24, 2019

Conversation

parthpp
Copy link
Contributor

@parthpp parthpp commented Jun 20, 2019

@Flet @wooorm
Github treats apostrophe as empty string. Hence, slugger should do the same. We have a requirement were we are consuming github-slugger and lack of this behaviour is causing issues. Hence, we would greatly appreciate if you could review and merge this PR soon. Thanks! 🙃

Copy link
Collaborator

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

The code here looks good, but it should come with a test, and it would be nice to also have it in a markdown file here, so that we can always check if it’s still correct (GH has changed their algorithm before)

Github treats apostrophe as empty string. Hence, slugger should do the same.
@parthpp parthpp force-pushed the addApostropheToSpecials branch from a0e5491 to 1bb4a9a Compare June 21, 2019 14:45
@parthpp
Copy link
Contributor Author

parthpp commented Jun 21, 2019

@wooorm Thanks for the review. I have added the test. I have also modified characters.md to include this case. Please review and merge 🙃.

@wooorm
Copy link
Collaborator

wooorm commented Jun 22, 2019

@parthpp Thanks, looks good!

@parthpp
Copy link
Contributor Author

parthpp commented Jun 24, 2019

When can we expect this to be merged and available on NPM?

@Flet Flet merged commit f486539 into Flet:master Jun 24, 2019
@Flet
Copy link
Owner

Flet commented Jun 25, 2019

@wooorm (or anyone) thoughts on if this should be patch or minor?

The interface to use it is the same, but the behavior differs slightly since its filtering apostrophe. It fixes a "bug" in that it mirrors GitHub behavior accurately, but folks may be caught off guard by the change in behavior.

Apologies if I seem overly cautious. The weekly npm downloads for this package are way higher than I remember! 😅

@wooorm
Copy link
Collaborator

wooorm commented Jun 25, 2019

I’d go with major, as these values are shown on websites and the like. So links will change.

Can we maybe check if eb4b887 is still needed?

@Flet
Copy link
Owner

Flet commented Jun 25, 2019

Yes I can find time to do this in the next day. And I can check out the other open issue too I suppose #22 :) May as well get things resolved if we do a major!

@wooorm
Copy link
Collaborator

wooorm commented Jun 25, 2019

Sweet!

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.

3 participants