ppannuto / python-titlecase

Python library to capitalize strings as specified by the New York Times Manual of Style
MIT License
244 stars 36 forks source link

#82: regex-only escape not supported in re fallback module #83

Closed fireundubh closed 3 years ago

fireundubh commented 3 years ago

Addressing #82:

All current tests pass. Whether existing tests cover the Unicode issues that the regex-only \p escape was intended to address is another matter. The author of #46 should be able to provide insight into whether the existing tests cover those issues.

brocksam commented 3 years ago

Excellent work, @fireundubh, thank you!

The current tests do include examples with Unicode characters (#46 added the examples suggested in #33 where this was originally raised).

Play devil's advocate, would we be better off keeping the package's logic the same in the cases where regex is available and falling back to using re and the approach reimplemented in this PR if it is not? #46 and regex may well perform slightly different in edge cases that aren't in the current tests and if we keep the package functioning identically for instances where regex can be found then we don't risk breaking functionality for any downstream users.

I've added #84 which adds a simple GitHub Actions workflow to set up automated running of the tests using a configuration without regex and also a configuration with regex. Merging #84, in conjunction with this PR (#83), should help ensure that Titlecase continues to work with re as a fallback for regex going forward.

brocksam commented 3 years ago

This PR is now superseded by #84, with all credit to @fireundubh.

ppannuto commented 3 years ago

Closing in favor of #84.