Conversation
| ); | ||
| }); | ||
|
|
||
| it('should parse a successful response with only one repo', () => { |
There was a problem hiding this comment.
I could have edited an existing test instead of adding a new one, but I liked the idea of just testing this one specific thing 🧪 (Happy either way though!)
There was a problem hiding this comment.
That's ok. Testing one thing at a time is good testing practice 👍
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
The number of typos 🙈
Changes LGTM
Could go further if you wanted to and apply the same change to the ${invalid_repositories.length} repositories were invalid and similar code just a few lines below. Maybe the code to make the "N repositor(y/ies)" message could be a reusable method. Also happy to go with this change and look at the other bits later if that's easier.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
LGTM 👍 🚢 🇮🇹
Up to you if you want to get a second opinion, since I helped write one of the commits 🤷
| logMessage += `:${eol}${cutoff_repositories.join(', ')}`; | ||
| if (cutoff_repositories_count !== cutoff_repositories.length) { | ||
| logMessage += `${eol}...${eol}And ${cutoff_repositories_count - cutoff_repositories.length} more repositrories.`; | ||
| logMessage += `${eol}...${eol}And ${cutoff_repositories_count - cutoff_repositories.length} more repositories.`; |
There was a problem hiding this comment.
I think this is right, please check my syntax (both English and Typescript).
| logMessage += `${eol}...${eol}And ${cutoff_repositories_count - cutoff_repositories.length} more repositories.`; | |
| const moreRepositories = cutoff_repositories_count - cutoff_repositories.length; | |
| logMessage += `${eol}...${eol}And ${moreRepositories} more ${pluralizeRepositories(moreRepositories)}.`; |
There was a problem hiding this comment.
Good point, although this could print 42 more 42 repositories if we're not careful. Perhaps something like
| logMessage += `${eol}...${eol}And ${cutoff_repositories_count - cutoff_repositories.length} more repositories.`; | |
| const moreRepositories = cutoff_repositories_count - cutoff_repositories.length; | |
| logMessage += `${eol}...${eol}And another ${pluralizeRepositories(moreRepositories)}.`; |
There was a problem hiding this comment.
Good spot, thanks both! Updated in latest commit 🦒
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Thanks for updating again 🥇
3 participants
Very minor, but the message
Successfully scheduled runs on 1 repositories.has been bugging me for a while 😅Just a bit of minor polish to pluralize (and singularize?) "repositories" appropriately! Happy to change if there are nicer ways of doing this 😀
(Also fixes 2 unrelated typos while I'm here)
Checklist
ready-for-doc-reviewlabel there.