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.
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
Add indicator native range year #65
Add indicator native range year #65
Changes from 8 commits
574f84b
d7373b9
c2ed621
2879cf5
2e5f002
215f434
614106d
323d1a5
7c8ff30
c4bd355
09958da
b4f8fca
6878392
2ae86fd
eafc40e
9d0eacc
b0e064b
66a8584
2d280b5
55678f9
655896b
84b14ad
405c0c1
36609c0
731271f
81a4952
bc4671b
946b1b2
5f247e3
b6305f5
1f49274
42fbd0a
29bc260
2b8f864
824f2c8
25fc430
947e0ea
ddbe7bd
11652f8
06b25c0
3766a36
5727ec5
24fc728
f7bc6d1
84c27d1
7f422a1
cb57365
4662830
e78d77a
3655230
0bfc5e5
3696004
244cc1c
ef8266a
e5e361d
bd09e4f
47f978f
b301f01
4ce92ef
e500397
f68ac0a
2610334
43a5c84
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like you use links for more information about code in documentation 👍
But if I compile the documentation via
devtools::document()
I get:This means the link is broken. R thinks your link is in trias package. Is
grofwild
a package? Then you can use something like this\code{\link[MASS]{abbey}}
? But I don't think it is a package. Then maybe a classic Markdown-style link solves the problem, e.g.:This vignette is very helpful: https://cran.r-project.org/web/packages/roxygen2/vignettes/rd-formatting.html#links
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied that part from @eadriaensen => should I include her as contributor as well ?
reportingGrofwild is a package, yes! However it is not published to cran or any other package repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@damianooldoni the link to the reporting-grofwild function has been rewriten but I don't know how to check if its working.
The installation of the modified trias package runs without new errors (expect the error described here #65 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comment above about link to
countYearProvince
, inheriting parameters doesn't work either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this line, will have to test if an alternative should be found !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed INBOtheme from trias functions as I got some troubles with it. Moreover, you don't use
inbo.2015.colours
explicitly in the code. To use INBOtheme colors, just load INBOtheme colors before using this function. This is how I do it in Rmd files of indicators repo where I use trias' visualization functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@damianooldoni should I write a .rmd for this function as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need and don't have to source within a package. Installing a package and loading it makes all functions available for tests as well.
Run
devtools::install()
and ´library(trias)(the latter command needed the first time) after you changed something in the functions and you have all functions ready to be used. So, in package development,
devtools::install()is your best friend,
source` your enemy 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I use devtools::install() with the script without plotly::layout() i get this error:
Note: possible error in 'layout(xaxis = list(title = x_lab, ': unused arguments (xaxis = list(title = x_lab, tickangle = "auto"), yaxis = list(title = y_lab, tickformat = ",d"), margin = list(b = 80, t = 100), barmode = ifelse(nlevels(summaryData$first_observed) == 1, "group", "stack")) at indicator_native_range_year.R:104
I suspected this was due to R thinking layout from the package graphics should be used, while the importFrom line clearly states layout should come from the plotly package (
@importForm plotly ggplotly, layout
).Adding
plotly::
before layout solves this problem. Any idea why this is ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somehow rebuilding the NAMESPACE (found out some functions I imported were missing) does not include plotly to the list. Maybe this is the cause??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a test. Testing plotly output is practically impossible or quite challenging, but you can test the data slot, and the fact that you get always a list and not something else... Writing tests is something which takes time, I know, but it helps making your code more robust and user friendly. For example, testing the input types, See other tests functions in trias package to understand what I mean. But I don't blame you if you don't want to spend time on the tests, I will do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all jokes aside, I simply used this to test if the function worked.