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

Conversation

@oscardelama
Copy link
Contributor

commented Jan 5, 2015

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.")

This comment has been minimized.

Copy link
@wch

wch Jan 6, 2015

Member

Should these be self$x?

R/print.R Outdated
@@ -70,12 +70,14 @@ object_summaries <- function(x) {
obj_names <- ls(x, all.names = TRUE)

values <- vapply(obj_names, function(name) {
obj <- x[[name]]
if (is.environment(x) && bindingIsActive(name, x)) "active binding"

This comment has been minimized.

Copy link
@wch

wch Jan 6, 2015

Member

I'd prefer that this has a style like:

if (is.environment(x) && bindingIsActive(name, x)) {
  "active binding"
} else {
  ...
}
@wch

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

Thanks! I have just a few minor comments.

@oscardelama

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

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

@wch

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

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

@oscardelama

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

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

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

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

@oscardelama

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

OK guys. Its ready.

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

wch added a commit that referenced this pull request Jan 6, 2015

Merge pull request #38 from oscardelama/master
Avoid reading active bindings when printing

@wch wch merged commit 369b1c0 into r-lib:master Jan 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@wch

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

Thanks!

@oscardelama

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

Thanks to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.