Issue https://github.com/ATFutures/calendar/issues/57 fix 🔧 #58
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
* Added person-object to description; Serkan Korkmaz * Fixed typo: iCalender to iCalendar
See issue #57. The function is a high-level stopifnot()- and tryCatch()-function wrapper built on {cli}-package.
* The function now uses the assert()-function instead of the stopifnot()-function * The function now uses inherits() instead of methods::is()
* ical()-function update: * The function now asserts passed arguments instead of storing the class. * All stop-controls have been replaced with assert. * Removed redundant assignment of column names to `n`. It is now directly passed into the control-flow. * description update: * methods have now been deleted from imports. Only a few functions were using it. * cli have been added to the import section * another typo in the title and description have been fixed. Was "iCalander" and now is "iCalendar"
|
Great stuff, will take a look! |
Robinlovelace
approved these changes
Aug 9, 2024
|
|
||
| return(x) | ||
|
|
||
| } | ||
|
|
||
| stopifnot(methods::is(object = x, class2 = "character") | methods::is(object = x, class2 = "list")) | ||
| assert( | ||
| inherits(x = x, what = "character") | inherits(x = x, what = "list"), |
| BEGIN = "VCALENDAR", | ||
| PRODID = "ATFutures/calendar", | ||
| VERSION = "2.0", | ||
| BEGIN = "VCALENDAR", |
There was a problem hiding this comment.
Why this extra whitespace?
There was a problem hiding this comment.
I don't know if it's redundant. But I thought it "looked better" honestly. I changed it from,
a = 2
aa = 2
aaa = 2
to
a = 2
aa = 2
aaa = 2
I am away so quick reply from me to say if it passes the tests (coverage of which I just checked is now deactivated on CodeCov) it is good enough for me. Thanks @serkor1 for the contribution. |
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.
Hi,
I have created a new
R-script calledutils, which contains theassert()-function. For some devs this is the standard file to put these kind of helper-functions in - this is at least what I do 😄Note
I have only added the functionality to functions that already used a mix of
stopifnotandstop. I haven't added this functionality on any other functions.I found two typos that I have corrected also. See commit e63371e and 24c1211.
The dependency on {methods} are gone, and a new dependency on {cli} is made. The {methods}-package were only used in two functions. So I believe this was a good choice.
I have run
devtools::check()and everything passes locally.Best,
@serkor1