Skip to content
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(proc-macros): re-export tokio for params parsing #1361

Merged
merged 7 commits into from
Apr 29, 2024

Conversation

niklasad1
Copy link
Member

@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
Member Author

@CodiumAI-Agent /review

@CodiumAI-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
11 checks passed
@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.

None yet

2 participants