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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trinistr
Copy link

@trinistr trinistr commented Jul 4, 2025

Fixes a couple of mistakes in documentation:

  • #modify's documentation very incorrectly said that it returns modified value, when that's not the case;
  • class's docs referenced #mutate instead of #modify;
  • typo in a comment.

Additionally, links to README and CHANGELOG are fixed in contributing guidelines, previously they linked to non-existent files.

@@ -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).
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

/README.md doesn't seem to work, on GitHub at least, same below of course

# 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

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.

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