fix: getPeers size check #2408
fix: getPeers size check #2408
Conversation
WalkthroughThe Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @pthmas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a targeted bug fix to the getPeers utility function, improving its robustness when handling empty input strings. The change ensures that an empty seed string correctly results in no peer IDs being returned, preventing a subtle parsing error that could occur with strings.Split.
Highlights
- Bug Fix: Peer ID Parsing: Addressed a bug in the
getPeersfunction where an empty input string (seeds) was incorrectly processed due to the behavior ofstrings.Split("", ",")returning[""]. - Improved Robustness: Ensured that the
getPeersfunction correctly handles an emptyseedsstring by returning an empty list of peer IDs, preventing an edge case where an empty string would be mistakenly treated as a valid, albeit empty, peer address.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request fixes a bug in the getPeers function where the strings.Split function was being called before checking if the seeds string was empty. This could lead to a slice of length 1 containing an empty string, which would cause issues later on. The fix is to check if the seeds string is empty before calling strings.Split.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2408 +/- ##
==========================================
+ Coverage 73.05% 73.95% +0.89%
==========================================
Files 67 67
Lines 6377 6642 +265
==========================================
+ Hits 4659 4912 +253
- Misses 1320 1327 +7
- Partials 398 403 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pkg/sync/sync_service.go
Outdated
| var peerIDs []peer.ID | ||
| sl := strings.Split(seeds, ",") | ||
| if len(sl) == 0 { | ||
| if len(seeds) == 0 { |
There was a problem hiding this comment.
this checks if the string has anything (!= ""). would seeds != "" be the same then?
There was a problem hiding this comment.
Yes correct, let me change that quickly. Your alternative makes it more readable.
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
left a question. its probably cheaper to check "" instead of len == 0
pthmas
force-pushed
the
pierrick/fix-peer-size-check
branch
from
5902ff4 to
1a96a91
Compare
This reverts commit b03dac8.
|
|
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview Duplicate of #2408, was reverted in #2394 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. Ex: Closes #<issue number> --> Co-authored-by: pthmas <9058370+pthmas@users.noreply.github.com>
Overview
While trying to fix the testapp tutorial from the docs I stumbled upon that issue.
By checking
[len(seeds) == 0]before splitting, you avoid the common pitfall where [strings.Split("", ",")]returns a slice of length 1 containing an empty string[""].Summary by CodeRabbit