Skip to content
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

Avoid reading active bindings when printing #38

Merged
merged 2 commits into from
Jan 6, 2015
Merged

Avoid reading active bindings when printing #38

merged 2 commits into from
Jan 6, 2015

Conversation

oscardelama
Copy link
Contributor

Including the fix and extending a test case

stop("Sorry this is a read-only variable.")
else {
# In "getter" role
if (x < 0) stop("The requested value is not available.")
Copy link
Member

Choose a reason for hiding this comment

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

Should these be self$x?

@wch
Copy link
Member

wch commented Jan 6, 2015

Thanks! I have just a few minor comments.

@oscardelama
Copy link
Contributor Author

yeah, there's more than one way to skin a cat (ugly proverb). Anyway, I am glad you like it.

@wch
Copy link
Member

wch commented Jan 6, 2015

@oscardelama Once the comments are addressed (you can add another commit and push), I'll merge this PR.

@oscardelama
Copy link
Contributor Author

Oops. Sorry if I look rather slow. I am a noob in this matters. This is my first pull request and I didn't figure out what was the next step.

I understand I should align my fork with your comments and that's it. Am I right?

@gaborcsardi
Copy link
Member

@oscardelama Update your code, commit and push. That's all you need to do.

@oscardelama
Copy link
Contributor Author

OK guys. Its ready.

I love the palindromic commit number '111a111' !!

wch added a commit that referenced this pull request Jan 6, 2015
Avoid reading active bindings when printing
@wch wch merged commit 369b1c0 into r-lib:master Jan 6, 2015
@wch
Copy link
Member

wch commented Jan 6, 2015

Thanks!

@oscardelama
Copy link
Contributor Author

Thanks to you!

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.

3 participants