Skip to content

Remove defunct test for non implementation of sync - #39

Merged
jonhoo merged 8 commits into
jonhoo:masterfrom
ptravers:master
Feb 18, 2020
Merged

Remove defunct test for non implementation of sync#39
jonhoo merged 8 commits into
jonhoo:masterfrom
ptravers:master

Conversation

@ptravers

@ptravers ptravers commented Jan 19, 2020

Copy link
Copy Markdown
Contributor

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

jonhoo commented Jan 20, 2020

Copy link
Copy Markdown
Owner

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

ptravers commented Jan 28, 2020

Copy link
Copy Markdown
Contributor Author

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

jonhoo commented Jan 28, 2020

Copy link
Copy Markdown
Owner

No worries, life gets busy some times!

I think ReadHandle should be Send, but not Sync.

Comment thread src/lib.rs Outdated
@codecov

codecov Bot commented Feb 2, 2020

Copy link
Copy Markdown

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

ptravers commented Feb 2, 2020

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/lib.rs
Comment thread src/lib.rs
@jonhoo

jonhoo commented Feb 18, 2020

Copy link
Copy Markdown
Owner

Beautiful, thank you!

@jonhoo
jonhoo merged commit b8c4f49 into jonhoo:master Feb 18, 2020
jonhoo pushed a commit that referenced this pull request Apr 4, 2026
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5.5.1 to 5.5.2.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@5a10915...671740a)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-version: 5.5.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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