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

chore: apply suggestions proposed by clippy #328

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

aleksuss
Copy link
Contributor

The motivation to propose these changes is to add the possibility of using this crate in projects which are checked with clippy with lints all and nursery.

@aleksuss aleksuss mentioned this pull request Oct 18, 2023
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing this up, but we need to understand motivation here. There are some obviously good changes, but there are also a lot of controversial ones, especially around promoting pub(crate) to pub. Please, provide more details in the comments

workspaces/src/network/config.rs Outdated Show resolved Hide resolved
workspaces/src/network/sandbox.rs Outdated Show resolved Hide resolved
workspaces/src/network/variants.rs Outdated Show resolved Hide resolved
workspaces/src/result.rs Outdated Show resolved Hide resolved
workspaces/src/rpc/client.rs Outdated Show resolved Hide resolved
workspaces/src/rpc/client.rs Show resolved Hide resolved
workspaces/src/rpc/client.rs Outdated Show resolved Hide resolved
workspaces/src/worker/impls.rs Show resolved Hide resolved
@aleksuss
Copy link
Contributor Author

aleksuss commented Oct 23, 2023

As described in the title, the changes have been proposed by the clippy linter. The point why clippy suggests making some methods and constants public is because they are defined in private modules, and these changes don't affect public API. And about main motivation. Now I can't bump version of this crate in my project because after bumping I get compiling errors:

note: future is not `Send` as this value is used across an await
   --> /Users/xxx/.cargo/registry/src/index.crates.io-6f17d22bba15001f/near-workspaces-0.8.0/src/rpc/patch.rs:289:81
    |
288 |                     AccountUpdate::FromCurrent(f) => {
    |                                                - has type `aurora_engine_types::Box<dyn std::ops::Fn(near_workspaces::types::account::AccountDetails) -> near_workspaces::types::account::AccountDetailsPatch>` which is not `Send`
289 |                         let account = self.worker.view_account(&self.account_id).await?;
    |                                                                                 ^^^^^^ await occurs here, with `f` maybe used later
290 |                         f(account)
291 |                     }
    |                     - `f` is later dropped here
    = note: `dyn std::ops::Fn(near_workspaces::types::account::AccountDetails) -> near_workspaces::types::account::AccountDetailsPatch` doesn't implement `std::marker::Send`
note: future is not `Send` as this value is used across an await
   --> /Users/xxx/.cargo/registry/src/index.crates.io-6f17d22bba15001f/near-workspaces-0.8.0/src/rpc/patch.rs:289:81
    |
284 |             for update in self.account_updates {
    |                 ------ has type `near_workspaces::rpc::patch::AccountUpdate` which is not `Send`
...
289 |                         let account = self.worker.view_account(&self.account_id).await?;
    |                                                                                 ^^^^^^ await occurs here, with `update` maybe used later
...
293 |             }
    |             - `update` is later dropped here
    = note: `(dyn std::ops::Fn(near_workspaces::types::account::AccountDetails) -> near_workspaces::types::account::AccountDetailsPatch + 'static)` doesn't implement `std::marker::Send`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send

@frol
Copy link
Collaborator

frol commented Oct 29, 2023

@aleksuss The compilation error due to the Send + Sync is real and indeed has to be addressed (please, also add a test).

I have disabled clippy::missing_const_for_fn and clippy::redundant_pub_crate lints as they produce false positives:

#![allow(clippy::missing_const_for_fn, clippy::redundant_pub_crate)]

Could you, please, revert the changes that added const fn and change visibility from pub(crate) to pub, and resolve merge conflicts?

@aleksuss aleksuss force-pushed the chore/aleksuss/clippy branch from 7b33d27 to c3f7782 Compare October 30, 2023 11:18
workspaces/src/rpc/client.rs Outdated Show resolved Hide resolved
@frol frol enabled auto-merge (squash) October 30, 2023 17:22
@frol
Copy link
Collaborator

frol commented Oct 30, 2023

@aleksuss Does your code work if you compile it using your forked near-workspaces?

@frol frol merged commit ff2bd56 into near:main Oct 30, 2023
6 checks passed
@aleksuss
Copy link
Contributor Author

@frol yes, now it compiles without errors.

@frol
Copy link
Collaborator

frol commented Oct 30, 2023

@aleksuss near-workspaces 0.9 release is published! Thank you for contributing!

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