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

Remove defunct test for non implementation of sync #39

Merged
merged 8 commits into from
Feb 18, 2020

Conversation

ptravers
Copy link
Contributor

@ptravers ptravers commented Jan 19, 2020

Hey I was looking through the implementation of this for a separate project and noticed this test which seems to be no longer necessary. I hope this is helpful!

The rust doc issue is now resolved with rust-lang/rust#47833

The documentation in cargo appears to reflect this as Sync is marked !Sync
https://docs.rs/evmap/4.1.1/evmap/struct.ReadHandle.html#impl-Sync

@ptravers ptravers requested a review from jonhoo January 19, 2020 16:12
@jonhoo
Copy link
Owner

jonhoo commented Jan 20, 2020

Ah, the intention behind having a test was that I don't always remember to check the documentation with every change that I make. I do run the test suite each time though, so if it could catch ReadHandle accidentally becoming Sync, that'd be much better. The test seems to currently be commented out though, and I forget exactly why. Maybe you could look into a way to bring the test back? That'd be super helpful!

@ptravers
Copy link
Contributor Author

ptravers commented Jan 28, 2020

Hey sorry this totally slipped by me in a slew of github comments. Sure happy to look at that. Right now it won't compile. Happy to take a look at writing an assertion that it will fail to compile due to the lack of Send or investigating a different solution for checking for the lack of Send. I think right now the compiler spews as RefCell cannot be passed to a new thread without Send implemented? But I will check a little later today!

Edit: Whoops totally flipped Sync and Send in my head!

@jonhoo
Copy link
Owner

jonhoo commented Jan 28, 2020

No worries, life gets busy some times!

I think ReadHandle should be Send, but not Sync.

src/lib.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 2, 2020

Codecov Report

Merging #39 into master will increase coverage by 4.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   78.65%   82.73%   +4.08%     
==========================================
  Files           7       11       +4     
  Lines         684      950     +266     
==========================================
+ Hits          538      786     +248     
- Misses        146      164      +18
Impacted Files Coverage Δ
src/lib.rs 58.82% <ø> (ø) ⬆️
tests/lib.rs 98% <0%> (-0.61%) ⬇️
src/shallow_copy.rs 58.82% <0%> (ø) ⬆️
src/read.rs
src/read/mod.rs 79.71% <0%> (ø)
src/read/read_ref.rs 89.28% <0%> (ø)
src/values.rs 72.22% <0%> (ø)
src/read/factory.rs 0% <0%> (ø)
src/read/guard.rs 90.9% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3862349...9ae4405. Read the comment docs.

@ptravers
Copy link
Contributor Author

ptravers commented Feb 2, 2020

hey I think this is now doing what we discussed! sorry for the delay!

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Feb 18, 2020

Beautiful, thank you!

@jonhoo jonhoo merged commit b8c4f49 into jonhoo:master Feb 18, 2020
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