-
Notifications
You must be signed in to change notification settings - Fork 724
Option mode #39
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
Option mode #39
Conversation
WalkthroughThis pull request introduces modifications across multiple files. A new entry for Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Nitpick comments (7)
examples/everything/main.go (1)
424-424: Optional: Consider specifying custom options.When creating the SSE server, you might want to pass configuration options (e.g.,
WithBaseURL) to better control endpoints or logging. As it stands, the default values suffice, but consider if your application needs more explicit settings.server/sse_test.go (3)
237-237: Consistency check.This instantiation duplicates the same base URL used above. Consolidating it in a shared helper or constant could reduce repetition across your tests.
291-291: Consistent test pattern.Again, the same functional option is used, which is fine for clarity. If multiple URLs are tested, consider centralizing them for maintainability.
400-400: Assert contextual information in test.You verify the status code for an accepted request. This is appropriate for the SSE-based workflow. As an optional improvement, consider also verifying any expected response body content when feasible.
server/sse.go (3)
23-25: Documentation improvement.The description for SSEServer is concise and clear. Consider briefly mentioning usage scenarios or typical flows (e.g., how to set up endpoints and send events) for more context.
37-42: Base URL setting.This option is straightforward. If your SSE server also supports relative endpoints, consider validating or normalizing the base URL input.
44-49: Message endpoint setter.No issues here. Just be mindful of leading/trailing slashes, as normalizing them can prevent accidental double slashes in paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)examples/everything/main.go(1 hunks)server/sse.go(3 hunks)server/sse_test.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (9)
examples/everything/main.go (1)
417-417: Consider consolidating or aliasing flags.You declare two flags (“-t” and “-transport”) that set the same variable. This can be intentional for aliasing, but it might also cause confusion for users if it's unintentional. Ensure that both flags are truly needed, or remove one to avoid confusion.
server/sse_test.go (2)
19-19: Good use of functional options.Using
WithBaseURL("http://localhost:8080")during tests is clear and aligns with the flexible options approach in your SSE server.
339-339: Using defaults without options.Here, you're testing the default server configuration. This is a good pattern to ensure coverage of the no-options scenario.
server/sse.go (6)
26-32: Sync.Map usage is acceptable.Storing sessions in a sync.Map is fine for concurrency. However, ensure your code properly handles session lifecycle and potential memory growth. Consider bounding session durations or removing stale sessions.
34-35: Functional options pattern is well-established.Defining an
Optiontype is a clean approach. This makes the code more configurable and readable.
51-56: SSE endpoint setter.Similarly, be aware of possible conflicts if two endpoints are set to the same path. Consider warnings or checks if that ever occurs.
58-63: Passing custom HTTP server.This is helpful for advanced use cases (TLS, custom middlewares). It’s good that you fall back to your own server if none is supplied.
65-79: Comprehensive constructor.The default endpoints (“/sse” and “/message”) are sensible and align with typical SSE usage. The functional option loop is cleanly applied.
305-307: ServeHTTP path handling.The new switch cases correctly delegate to
handleSSEandhandleMessageusing configured endpoints. Ensure that these match any user-configured routes if multiple SSE servers or prefix-based routing is used elsewhere.
|
@a67793581 tests are failing |
ok! i get ,i fix it. |
Summary by CodeRabbit