Skip to content

fix(proc-macros): re-export tokio for params parsing#1361

Merged
niklasad1 merged 7 commits intomasterfrom
na-tests-proc-macro-core
Apr 29, 2024
Merged

fix(proc-macros): re-export tokio for params parsing#1361
niklasad1 merged 7 commits intomasterfrom
na-tests-proc-macro-core

Conversation

@niklasad1
Copy link
Contributor

@niklasad1 niklasad1 commented Apr 29, 2024

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

@niklasad1 niklasad1 requested a review from a team as a code owner April 29, 2024 10:00
@niklasad1
Copy link
Contributor Author

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link

PR 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

Code feedback:
relevant filecore/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 linepub use tokio;

relevant filecore/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 fileproc-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 linelet tokio = self.jrps_server_item(quote! { core::__reexports::tokio });

relevant filetests/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)]


✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

  • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

@niklasad1 niklasad1 changed the title fix(proc-macros): re-export tokio and tests fix(proc-macros): re-export tokio for params parsing Apr 29, 2024
@niklasad1 niklasad1 merged commit c908eeb into master Apr 29, 2024
@niklasad1 niklasad1 deleted the na-tests-proc-macro-core branch April 29, 2024 10:29
niklasad1 added a commit that referenced this pull request Apr 29, 2024
* fix(proc-macros): re-export tokio and tests

* Update tests/proc-macro-core/src/lib.rs

* Update tests/proc-macro-core/Cargo.toml

* Update core/src/lib.rs

* Update tests/proc-macro-core/src/lib.rs

* tests: custom params types

* unify params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants