Page MenuHomePhabricator

[lib] Write some tests for `text-utils: pluralizeAndTrim(...)`
ClosedPublic

Authored by atul on Jun 22 2022, 1:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Thu, Oct 31, 11:53 AM
Unknown Object (File)
Oct 6 2024, 11:30 AM
Subscribers

Details

Summary

Did pluralize(...) and trim(...) so might as well do pluralizeAndTrim(...)

Test Plan

Ran tests

Diff Detail

Repository
rCOMM Comm
Branch
june21 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested review of this revision.Jun 22 2022, 1:13 PM
This revision is now accepted and ready to land.Jun 22 2022, 1:27 PM
This revision now requires review to proceed.Jun 22 2022, 1:47 PM

It might be worth adding a test where a length of a single item is higher than the limit

This revision is now accepted and ready to land.Jun 23 2022, 2:48 AM

rebase after cherrypicking and before addressing feedback

split string literals to ensure all lines <= 80 characters

In D4329#122351, @palys-swm wrote:

It might be worth adding a test where a length of a single item is higher than the limit

Done, added the "should trim the pluralized string to fit within maxLength" test to handle that case

lib/utils/text-utils.js
43 ↗(On Diff #13743)

@atul This change significantly alters the behavior described in a diff that introduced this code https://phab.comm.dev/D698. We need to make sure that the new behavior is better before changing it. Do you think the new behavior is better? At least we should check how it looks like in our UI - in this case it probably can be tested by checking how inline sidebar looks like when only one user with a long name responded to it.
As a side note, it's always a good idea to check git blame and see in the original diff what was the intention.

lib/utils/text-utils.js
43 ↗(On Diff #13743)

We need to make sure that the new behavior is better before changing it. Do you think the new behavior is better?

I think so? Previously, if the 3-noun and 2-noun representation didn't fit within maxLength, we would go with the 1-noun representation even if it exceeded maxLength. Given that the name of the function is pluralizeAndTrim and that maxLength is an argument, I thought that the original intention was to cap the number of characters to maxLength.

I think this was suggested in D698 as well:

How does this look if it the result of pluralize is really long? If it doesn't look good, we could try to use something different instead of pluralize that tries to limit character length.


At least we should check how it looks like in our UI - in this case it probably can be tested by checking how inline sidebar looks like when only one user with a long name responded to it.

Yeah you're right, I should have looked at different usages in the UI instead of looking at this function in isolation. I'll see what InlineSidebar looks like when a user with a long username responds, and I'll add a response here with what I find.

As a side note, it's always a good idea to check git blame and see in the original diff what was the intention.

Agreed, I should have done that as well.