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

remove_s4_classes() respecting inheritance #849

Merged
merged 8 commits into from
Sep 3, 2015

Conversation

famuvie
Copy link

@famuvie famuvie commented Jun 11, 2015

prevents derived classes to remain orphan
Fixes #848

prevents derived classes to remain orphan
Fixes r-lib#848
@famuvie
Copy link
Author

famuvie commented Jun 11, 2015

Mmm... contrary to #848, with this reverse removing of classes the error arises when the derived class is named 'splines'. What is it special with this name?

@famuvie
Copy link
Author

famuvie commented Jun 12, 2015

Ok. There is nothing particular with the class name splines except for the fact that alphabetically comes after my_class. The current devtools version removes classes by alphabetical order which happen to coincide with inheritance hierarchy (i.e. my_class is extended by splines).

I needed to write a small routine to sort the classes defined in the package, so that extended classes are before their respective extensions. With this approach I think the problem is solved.

@famuvie famuvie changed the title remove_s4_classes() in reverse order remove_s4_classes() respecting inheritance Jun 12, 2015
@hadley
Copy link
Member

hadley commented Jun 12, 2015

@wch happen to remember an elegant way to do this?

@hadley
Copy link
Member

hadley commented Jun 15, 2015

Ideally you'd do a topological sort on the S4 dependency graph before deleting

@famuvie
Copy link
Author

famuvie commented Jun 15, 2015

Right. My solution worked for me, but not necessarily in general.
Implemented a topological sorting. Not very fast in R, but graphs of classes should not be huge.

@hadley
Copy link
Member

hadley commented Jun 19, 2015

Could you please add some unit tests for sort_s4classes()?

@famuvie
Copy link
Author

famuvie commented Jun 19, 2015

sure... in a couple of weeks.

@famuvie
Copy link
Author

famuvie commented Jul 13, 2015

Done, with some modifications thanks to the testing.
Incidentally, this fixes one previously failing check that was commented out in test-s4-unload.R.

}

## Matrix of classes in columns, extending classes in rows
extended_classes <- vapply(classes,
Copy link
Member

Choose a reason for hiding this comment

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

Please indent arguments by only two spaces

@hadley
Copy link
Member

hadley commented Sep 2, 2015

Could you please also add a bullet to NEWS?

@@ -0,0 +1,11 @@
Package: testS4sort
Title: Test package for sorting S4 classes
License: GPL-3
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be best to just take out the License field here. Or it should be the same license as devtools itself.

@famuvie
Copy link
Author

famuvie commented Sep 3, 2015

Done.

hadley added a commit that referenced this pull request Sep 3, 2015
remove_s4_classes() respecting inheritance. Closes #848
@hadley hadley merged commit e2f3746 into r-lib:master Sep 3, 2015
@hadley
Copy link
Member

hadley commented Sep 3, 2015

Thanks!

@famuvie famuvie deleted the remove_s4_classes_with_inheritance branch September 3, 2015 13:45
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