Remove #[async_trait] where possible
#1055
Conversation
|
|
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 3 discussions need to be resolved
-- commits line 2 at r1:
nit: Please reference ticket in commit.
nativelink-util/src/health_utils.rs line 104 at r1 (raw file):
/// A default implementation is provided for the `check_health` function /// that returns healthy component. #[async_trait] // FIXME remove this if/when traits containing async fn can be used as trait objects
nit: This comment doesn't have any value. There's no timeline on when/if they will ever do this, so lets just keep using it as is without a comment.
nativelink-util/src/store_trait.rs line 153 at r1 (raw file):
/// A key that has been subscribed to in the store. This can be used /// to wait for changes to the data for the key. #[async_trait] // FIXME remove this if/when traits containing async fn can be used as trait objects
ditto.
There was a problem hiding this comment.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Installation / macos-13, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, windows-2022 / stable, and 3 discussions need to be resolved
There was a problem hiding this comment.
I've made the requested changes.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 1 discussions need to be resolved
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Please reference ticket in commit.
done
nativelink-util/src/health_utils.rs line 104 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: This comment doesn't have any value. There's no timeline on when/if they will ever do this, so lets just keep using it as is without a comment.
done
nativelink-util/src/store_trait.rs line 153 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
ditto.
done
There was a problem hiding this comment.
Reviewed 11 of 13 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved
There was a problem hiding this comment.
Reviewable status: 3 of 1 LGTMs obtained, and 1 discussions need to be resolved
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 3 of 1 LGTMs obtained, and 1 discussions need to be resolved
-- commits line 2 at r2:
nit: Please use format:
Remove `#[async_trait]` where possible
closes #620
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 3 of 1 LGTMs obtained, and 1 discussions need to be resolved
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Please use format:
Remove `#[async_trait]` where possible closes #620
nit to the nit:
Should actually have a capital C as in Closes #620 😛
There was a problem hiding this comment.
Reviewable status: 3 of 1 LGTMs obtained, and 1 discussions need to be resolved
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 3 of 1 LGTMs obtained, and 1 discussions need to be resolved
There was a problem hiding this comment.
Reviewable status: 3 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit to the nit:
Should actually have a capital C as in
Closes #620😛
done
Description
async fnand-> impl Traitis stable in traits. This uses it wherever possible. Due to trait bound requirements, all converted functions need-> impl Future + Send.Due to compiler limitations, namely
dyn Traitcompatibility, it is not possible at this time to convert the remaining uses without creating concrete types and implementingFutureby hand.Fixes #620
Type of change
Code simplification
How Has This Been Tested?
cargo checkpasses as expected;bazel test //...succeeds as well.Checklist
bazel test //...passes locallygit amendsee some docsThis change is