search

piotrmurach / strings

A set of useful functions for transforming strings.
MIT License
129 stars 12 forks source link

Fix Strings.truncate to return the correct length #18

Closed justingaylor closed 2 years ago

justingaylor commented 2 years ago

Describe the change

Fixes https://github.com/piotrmurach/strings/issues/17

Why are we doing this?

I believe we should be returning characters of N display width when specifying Strings.truncate(s, N).

Benefits

Strings.truncate will be more intuitive (e.g. follow the Principle of Least Surprise).

Drawbacks

It is possible that existing usages of this library are coded to expect the width of displayed characters to be strictly < truncate_at, but equal to it. In this case, they will get potentially an extra character.

If this is a concern, we could instead update the documentation here to specifying that truncate_at is not inclusive (e.g. Strings.truncate does not return a character at truncate_at width).

Alternatively, we could introduce an :inclusive option (defaulted to false) which would treat the behavior of truncate_at as inclusive when passed.

Requirements

Put an X between brackets on each line if you have done the item: [X] Tests written & passing locally? [] Code style checked? [X] Rebased with master branch? [] Documentation updated?

piotrmurach commented 2 years ago

Hi Justin,

I do apologise for not making this clearer in the readme that the intention is to use strings-truncation instead. I do hope this won't discourage you from contributing in the future.

justingaylor commented 2 years ago

@piotrmurach Thanks for the heads up! Would you like me to add something to the README here so it is clearer for others in the future? All I see currently is a reference to strings-truncate as being independently consumable, not that it is a replacement for this gem's truncate functionality. https://github.com/piotrmurach/strings#4-components

Happy to write up something! 😄

piotrmurach commented 2 years ago

@justingaylor You're right. We could add a note under the truncate section along the lines: "Please note this API will change in the next release and will be replaced by the strings-truncation component" or something similar.