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

ts-human-format-duration - use singular unit names when values are 1 #12

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

Conversation

contrapunctus-1
Copy link

Hope it's okay, I'm far from being an expert on macros 😅

@alphapapa
Copy link
Owner

Well:

  1. I'm not sure if it's worth adding this complexity. Sometimes this code could be called in tight loops (e.g. formatting a long list of items), and Elisp isn't that fast, so it could make a difference. Hard to say.

  2. You changed a test case, but you didn't add one for the former case (of plurals).

I guess it's somewhat a matter of style, so I'm not sure about this. But I appreciate your proposing it. What do you think?

@contrapunctus-1
Copy link
Author

contrapunctus-1 commented May 19, 2020

Hey 🙂

Can't say about performance, but it would certainly result in a reduction in code for me personally, since I wouldn't need to reimplement the same function to output a grammatically-correct string (which in my case is user-facing, and I'd like for it to be polished).

I had created additional test cases, but they failed because of the leap year problem, so I stashed them. Looking at the tests again just now, I realized the reason for using 'day instead of 'year, so I've pushed some tests.

Glad to be of help 😀

@alphapapa
Copy link
Owner

Thanks for your patience.

I think I'd prefer to gate this behind a flag, so the extra string operations can be omitted unless needed. Can you think of a good name for the flag?

@contrapunctus-1
Copy link
Author

Hm, I quite forgot about this. use-singular-units-p ? 🙂

@alphapapa alphapapa added this to the Future milestone Aug 22, 2022
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.

2 participants