-
Notifications
You must be signed in to change notification settings - Fork 211
CORE-2303 #7148
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
Conversation
Tie-breaker fix for CORE-2303
Only look at latest submission
| key: `${member.memberId || member.member || ''}`, | ||
| score: member[scoreKey], | ||
| tieBreaker: tieBreaker ? tieBreaker(member) : null, | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The tieBreaker function is called for every member, which could be inefficient if the function is computationally expensive. Consider caching the results if the tieBreaker values are reused or if the computation is costly.
| : submissions)[0] || null; | ||
| // Provisional ranks should be based solely on the most recent submission, | ||
| // not the best historical one. | ||
| const bestProvisionalScore = normalizeScoreValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The bestProvisionalScore is now based solely on the most recent submission rather than the best historical one. Ensure this change aligns with the intended business logic, as it could impact the ranking outcome.
| 'bestProvisionalScore', | ||
| 'provisionalRank', | ||
| { | ||
| tieBreaker: (entry) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 readability]
The tieBreaker function uses Number.POSITIVE_INFINITY for missing timestamps, which might not be intuitive. Consider using a more explicit value or documenting this behavior to avoid confusion.
Potential fix for ` vs. ``` in markdown rendering
| padding: 0 6px; | ||
| margin: 0; | ||
| display: inline; | ||
| white-space: pre-wrap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The change from white-space: pre to white-space: pre-wrap for code elements might affect how whitespace is handled within inline code blocks. Ensure this change is intentional and that it does not negatively impact the display of code snippets, especially if they rely on precise formatting.
No description provided.