search

vorner / arc-swap

Support atomic operations on Arc itself
Apache License 2.0
778 stars 31 forks source link

FIX: Use correct trait for display #22

Closed neoeinstein closed 5 years ago

neoeinstein commented 5 years ago

The current implementation incorrectly delegates Display on Guard to the Debug implementation. It should instead delegate to the underlying Display implementation.

vorner commented 5 years ago

Thank you for spotting this!

However, the problem is, this is technically a API-breaking change ‒ code that compiled previously could stop compiling. On the other hand, if people actually used this, they would have noticed, likely, like you did, so I'm inclined to just release it with the patch version update only. What do you think is the better way forward? Or, do you know of an easy way to run an update on all reverse dependencies and see if they compile and pass tests?

neoeinstein commented 5 years ago

I think that depends on how you see this. Yes, this would be a breaking change, but it is due to a bug in the API. I took a look at the list of crates that depend on arc-swap versions 0.4, and none of those listed attempt to use Display on an ArcSwap, so none seem to exercise this particular bug. I can certainly see incrementing the minor version to 0.5, but I figured this is a bug fix, which is why my change initially marks for 0.4.3. I'm happy to change it to 0.5 if you prefer.

vorner commented 5 years ago

Thank you for the research, I think this confirms my suspicion this is fine to go ahead with the patch version :-).