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

Support setMethodS3() and setConstructorS3() of R.methodsS3 and R.oo #525

Merged
merged 5 commits into from Oct 10, 2016
Merged

Support setMethodS3() and setConstructorS3() of R.methodsS3 and R.oo #525

merged 5 commits into from Oct 10, 2016

Conversation

@HenrikBengtsson
Copy link
Contributor

@HenrikBengtsson HenrikBengtsson commented Oct 10, 2016

R.methodsS3::setMethodS3(name, class, definition) defines S3 method name.class <- definition and R.oo::setConstructorS3(name, definition) defines R function name <- definition. With this patch, roxygen2 recognizes the above constructors accordingly.

For the records, the background for this PR is private email 'R.methodsS3 with roxygen2' on 2016-10-03 from @martynplummer to me and @hadley (cc: @Lucaweihs and Mathias Drton). The patch has been validated by me and independently by @martynplummer.

class <- as.character(call[[3]])
name <- paste(method, class, sep=".")
value <- get(name, env)
value <- standardise_obj(name, value, env, block)
Copy link
Member

@hadley hadley Oct 10, 2016

Choose a reason for hiding this comment

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

Maybe just move get(...) into the standarise_obj() call?

value <- get(name, env)
value <- standardise_obj(name, value, env, block)
object(value, name)

Copy link
Member

@hadley hadley Oct 10, 2016

Choose a reason for hiding this comment

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

Extraneous line here

@hadley
Copy link
Member

@hadley hadley commented Oct 10, 2016

Just a couple of tiny things; otherwise looks good. Can you please also add a bullet to NEWS?

@HenrikBengtsson
Copy link
Contributor Author

@HenrikBengtsson HenrikBengtsson commented Oct 10, 2016

Done.

Though, not sure if I PR's should be referenced in NEWS or just issues.

@@ -1,5 +1,8 @@
# roxygen2 5.0.1.9000

* S3 method declarations via setMethodS3() of R.methodS3 and function
declarations via setConstructorS3() of R.oo are now supported (#525).
Copy link
Member

@hadley hadley Oct 10, 2016

Choose a reason for hiding this comment

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

Can you include your github user name there too?

I don't have a firm policy on issues vs. PRs — but I usually cite the issue number if there is one, other the PR is fine.

@HenrikBengtsson
Copy link
Contributor Author

@HenrikBengtsson HenrikBengtsson commented Oct 10, 2016

Added user name.

@hadley hadley merged commit 24decf9 into r-lib:master Oct 10, 2016
1 check was pending
@hadley
Copy link
Member

@hadley hadley commented Oct 10, 2016

Thanks!

Lucaweihs added a commit to Lucaweihs/roxygen that referenced this issue Jul 14, 2017
…:setConstructorS3. This bug was introduced in code review of pull request r-lib#525 (the arguments to a call to standardise_obj were incorrectly changed).
hadley added a commit that referenced this issue Aug 21, 2017
…:setConstructorS3. This bug was introduced in code review of pull request #525 (the arguments to a call to standardise_obj were incorrectly changed). (#639)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants