fix(core): updates to SpelledNumbers #325
Conversation
- Fix comments - Change from suggesting if <= 10 to < 10 - Expand spelling function (actually works for 110 now) - Update tests accordingly
| "{}{}{}", | ||
| spell_out_number(parent).unwrap(), | ||
| if num <= 99 { '-' } else { ' ' }, |
There was a problem hiding this comment.
This is because numbers between twenty-one and ninety-nine should be hyphenated.
There was a problem hiding this comment.
Toss this info in a comment in the code.
| let n = 10u64.pow((num as f32).log10() as u32); | ||
| let parent = (num / n) * n; // truncate | ||
| let child = num % n; |
There was a problem hiding this comment.
This modification allows the truncation to extend to n digits, instead of just working for two digits as in the past.
This used to cause a stack overflow for n = 110. (and probably 101 to 109 as well)
| hundred if hundred % 100 == 0 => { | ||
| format!("{} hundred", spell_out_number(hundred / 100).unwrap()) | ||
| } |
There was a problem hiding this comment.
This logic should probably be expanded to include more suffixes for powers of 10 up to 10^19 so it will work for large numbers at the beginning of sentences.
(10^19 is the largest power of 10 a u64 could represent)
| lints.push(Lint { | ||
| span: number_tok.span, | ||
| lint_kind: LintKind::Readability, | ||
| suggestions: vec![Suggestion::ReplaceWith( | ||
| spell_out_number(number as u64).unwrap().chars().collect(), | ||
| )], | ||
| message: "Try to spell out numbers less than a hundred.".to_string(), | ||
| message: "Try to spell out numbers less than ten.".to_string(), |
There was a problem hiding this comment.
It is kinda funny that this is spelled out when under my change it wouldn't be suggested.
I've always worked on the principle of:
- Spelled out for < 10
- Numerals for > 10
- Whatever feels best for 10 in the moment
I am aware. This was planned.
It's quite closely based on the Chicago Style, with a few changes. Overall, this PR looks great! Merging... |
I wasn't able to find an "Automattic style guide" as referenced in recent
commits on this, but it is rather standard to spell out numbers under but not
including 10. This is what APA does, and I believe MLA as well.
Recent commits also didn't update doc comments to reflect new behavior, so I
fixed that.
One thing to keep in mind, but I don't think is implemented yet, is that
numbers beginning sentences should always be spelled out, so it would be best
to expand behavior to include this in further enhancements.