Skip to content

Fix mistakes in MVar documentation #1087

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 7, 2025
Merged

Fix mistakes in MVar documentation #1087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ There are a few guidelines which we follow when adding features. Consider that s

#### Write Documentation

Document any external behavior in the [README](README.md).
Document any external behavior in the [README](../README.md).

#### Commit Changes

Expand Down Expand Up @@ -106,7 +106,7 @@ git push origin my-feature-branch -f

#### Update CHANGELOG

Update the [CHANGELOG](CHANGELOG.md) with a description of what you have changed.
Update the [CHANGELOG](../CHANGELOG.md) with a description of what you have changed.

#### Check on Your Pull Request

Expand Down
8 changes: 4 additions & 4 deletions lib/concurrent-ruby/concurrent/mvar.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module Concurrent
# queue of length one, or a special kind of mutable variable.
#
# On top of the fundamental `#put` and `#take` operations, we also provide a
# `#mutate` that is atomic with respect to operations on the same instance.
# `#modify` that is atomic with respect to operations on the same instance.
# These operations all support timeouts.
#
# We also support non-blocking operations `#try_put!` and `#try_take!`, a
Expand Down Expand Up @@ -87,7 +87,7 @@ def borrow(timeout = nil)
@mutex.synchronize do
wait_for_full(timeout)

# if we timeoud out we'll still be empty
# If we timed out we'll still be empty
if unlocked_full?
yield @value
else
Expand Down Expand Up @@ -116,10 +116,10 @@ def put(value, timeout = nil)
end

# Atomically `take`, yield the value to a block for transformation, and then
# `put` the transformed value. Returns the transformed value. A timeout can
# `put` the transformed value. Returns the pre-transform value. A timeout can
# be set to limit the time spent blocked, in which case it returns `TIMEOUT`
# if the time is exceeded.
# @return [Object] the transformed value, or `TIMEOUT`
# @return [Object] the pre-transform value, or `TIMEOUT`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if instead the code should return the transformed value.
But looking at

concurrent-ruby/spec/concurrent/mvar_spec.rb

Lines 180 to 183 in c8f0bae

it 'returns the unmodified value' do
m = MVar.new(14)
expect(m.modify { |v| v + 2 }).to eq 14
end
it seems not, so indeed the doc change seems correct then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I noticed the problem because the documented behavior seemed weird, as getting previous value would be hard in many cases, and it did not follow the "set and get" pattern.

def modify(timeout = nil)
raise ArgumentError.new('no block given') unless block_given?

Expand Down