Closed niklasad1 closed 5 months ago
@CodiumAI-Agent /review
⏱️ Estimated effort to review [1-5] | 3, because the PR involves changes across multiple files including core library files and macro definitions, which requires careful consideration to ensure that the changes do not introduce any regressions or compatibility issues. The addition of a new test module increases the complexity slightly. |
🧪 Relevant tests | Yes |
🔍 Possible issues | Possible Bug: The macro `cfg_client_or_server` uses `#[cfg(any(feature = "client", feature = "server"))]` which might not correctly handle cases where both features are enabled or other feature-specific logic is required. |
🔒 Security concerns | No |
relevant file | core/src/lib.rs |
suggestion | Consider adding a comment explaining why `tokio` is conditionally included under `cfg_client_or_server`. This helps maintainers understand the context and necessity of this conditional compilation. [medium] |
relevant line | pub use tokio; |
relevant file | core/src/macros.rs |
suggestion | It might be beneficial to add a fallback or error handling mechanism within the `cfg_client_or_server` macro to manage unexpected conditions where neither 'client' nor 'server' features are enabled. [important] |
relevant line | #[cfg(any(feature = "client", feature = "server"))] |
relevant file | proc-macros/src/render_server.rs |
suggestion | Ensure that the re-exported `tokio` from `core::__reexports` does not lead to any unexpected behavior or conflicts, especially in environments where `tokio` might be independently required by other modules. [medium] |
relevant line | let tokio = self.jrps_server_item(quote! { core::__reexports::tokio }); |
relevant file | tests/proc-macro-core/src/lib.rs |
suggestion | Verify that the new test module `jsonrpsee-proc-macro-core` covers all new paths introduced by the PR, particularly the conditional re-export of `tokio` and the new macro definitions. [important] |
relevant line | #[rpc(client, server)] |
Follow-up on #1360 and I forgot that we use tokio in param parsing.
I also added a test crate to ensure that it compiles