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

Copy link
Author

@trinistr trinistr Jul 6, 2025

Choose a reason for hiding this comment

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

Hmm, weird, it did work for me when I viewed CONTRIBUTING.md in the repository.

Fixed.

# 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.

Copy link
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.

@trinistr trinistr force-pushed the fix-mvar-documentation branch from 492a58c to a58b093 Compare July 6, 2025 14:16
@trinistr trinistr force-pushed the fix-mvar-documentation branch from a58b093 to 2c815db Compare July 6, 2025 14:17
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