Implemented S7 by swapping the rate class and subclasses from S3
#1154
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Need to update tests, documentation. Note that order of arguments are different for subclasses in the current structure--change that.
1. Very ugly way to override super's max_times in rate_delay 2. Changed test cases a bit to correspond with new S7 syntax, but getting random errors so need to look into more
Couple caveats/notes: Needed to change a few tests to match new S7 syntax. Current method of overriding super's default values is inelegant (see `rate_delay`), but functional. Would like to change. Still needs some documentation.
Changed class construction to conform with new API. Removed default values in properties as they are listed in constructors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1129
Notes/caveats:
I changed some tests to account for the new syntax of S7 objects--eg. changing
$to@. Additionally, the new constructor forrateobjects is called justrate()instead ofnew_rate()so I also changed some variable names in the tests to not conflict with that.I don't like line 71 in rate.R. There, I have a subclass with a different default value of a property than the parent class. I want to override the super's default value for
max_times, but when I just includemax_timesas a property of the subclass with a different default, the constructed object's value goes to that of the parent. (3 instead of Infinity forrate_delay).I got around it by checking if
max_timeswas supplied as an argument and providing the correct default value if it wasn't.I didn't write change any documentation, so if that needs to be changed I will fix that. The changes are mainly internal, but it may be good to document what certain things are doing so that if S7 changes the purpose of the code can still be understood.
Mentioning @kbodwin since this PR is part of the awesome Independent Study: Advanced R with Dr Bodwin this quarter!