-
Notifications
You must be signed in to change notification settings - Fork 61
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
fix: issue 1381 using startWiths instead of substring for check stringg prefixess #1557
Conversation
Someone is attempting to deploy a commit to the quicksilver Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe recent change in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 2
linting Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1557 +/- ##
=======================================
Coverage 63.60% 63.60%
=======================================
Files 196 196
Lines 13779 13779
=======================================
Hits 8764 8764
Misses 4158 4158
Partials 857 857
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web-ui/utils/string.ts (1 hunks)
Additional comments not posted (1)
web-ui/utils/string.ts (1)
2-3
: Verify the correctness of the change.The change from
substring
tostartsWith
is correct and improves readability and performance. Ensure that all test cases cover this change and that no edge cases are missed.
if (denom.startsWith('Q') || denom.startsWith('AQ')) { | ||
return 'q' + denom.substring(1); | ||
} |
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.
Ensure consistent return format for formatQasset
.
The function formatQasset
is documented to return a string, but the format of the returned string varies depending on the condition. If denom
starts with "Q" or "AQ", it returns "q" + denom.substring(1)
, which alters the string. Otherwise, it returns denom
unchanged. Consider normalizing the return format or clarifying the function's purpose and expected behavior in the documentation.
Improve code formatting for better readability.
- if (denom.startsWith('Q') || denom.startsWith('AQ')) {
- return 'q' + denom.substring(1);
- }
+ if (denom.startsWith('Q') || denom.startsWith('AQ')) {
+ return 'q' + denom.substring(1);
+ }
Adding spaces around operators and after conditions, as well as semicolons at the end of statements, will enhance the readability and maintainability of the code.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if (denom.startsWith('Q') || denom.startsWith('AQ')) { | |
return 'q' + denom.substring(1); | |
} | |
if (denom.startsWith('Q') || denom.startsWith('AQ')) { | |
return 'q' + denom.substring(1); | |
} |
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.
small but nice improvement, thanks @tiendn
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Hey @faddat . Can u check this PR? |
The reason this PR hasn’t been merged is because it has highlighted that the function is broken (not by you, but was and is still fundamentally broken). I also wanted to check that this wasn’t fixed elsewhere in a recent PR, before merging it. So presently it is on hold. |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
OK bye @joe-bowman |
1. Summary
startWiths
look more clear than usesubString
, and have better performance.Fixes #1381
2.Type of change
3. Implementation details
use
startWiths
function4. How to test/use
5. Checklist
6. Limitations (optional)
7. Future Work (optional)