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

Fix a bunch of R CMD check warnings and notes #27

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Fix a bunch of R CMD check warnings and notes #27

merged 2 commits into from
Sep 18, 2023

Conversation

rempsyc
Copy link

@rempsyc rempsyc commented Sep 11, 2023

It seems like R CMD check was never run on this package 💀

I've fixed as much as I could. There are still one warning and one note:

── R CMD check results ─────────────────────────────────────────────────────────── ggflags 0.0.3 ────
Duration: 25.8schecking for missing documentation entries ... WARNING
  Undocumented data sets:
    '.flaglist'
  All user-level objects in a package should have documentation entries.
  See chapter 'Writing R documentation files' in the 'Writing R
  Extensions' manual.checking R code for possible problems ... NOTE
  makeContent.flag : <anonymous>: no visible binding for global variable
    '.flaglist'
  Undefined global functions or variables:
    .flaglist

0 errors| 1 warning| 1 noteError: R CMD check found WARNINGs
Execution halted

Exited with status 1.

Both seem related to documenting the dataset. Although it seems like I did document it, something does not seem to be working correctly. I have tried to use usethis::use_data() but this requires either having the original data object in the environment, or to be able to create it from scratch.

In theory, having the data object in the environment should be as simple as loading the lflags.Rda file. However, opening it for some obscure reason does not load it into the environment, both when double-clicking on the file manually or when using:

load(file = "data/lflags.Rda")
#> Warning in readChar(con, 5L, useBytes = TRUE): cannot open compressed file
#> 'data/lflags.Rda', probable reason 'No such file or directory'
#> Error in readChar(con, 5L, useBytes = TRUE): cannot open the connection
exists("lflags.Rda")
#> [1] FALSE

Created on 2023-09-10 with reprex v2.0.2

I wonder whether the file got corrupted over time or whether it is too old for the current R version. The file was last edited in 2020 by someone else, so that could be it also... Could you try to see if you are able to load the file in your environment at all?

Otherwise I would need to know how to regenerate the file to progress further. Thanks.


Attempts to close #26

@jimjam-slam
Copy link
Owner

Very possible—I never set up proper package infra for this package, just modified it to use SVGs!

lflags.rda is generated using store_flags.R. It requires that the source flag SVGs (currently from EmojiOne, but I'd like to migrate to the more current HatScripts/circle-flags at some point) be present in the same directory when running the script.

I recall having problems with {grImport2} last time I ran this, and that was what started me thinking that the SVG dependencies really ought to be updated, so that's likely where I've been blocked for a while!

@rempsyc
Copy link
Author

rempsyc commented Sep 16, 2023

The grConvert package is not available on CRAN, and it seems like the GitHub repository has been archived by the owner. remotes::install_github("sjp/grConvert") also doesn't work: Error: this package has a configure script. It probably needs manual configuration.

How can I install it?

@rempsyc
Copy link
Author

rempsyc commented Sep 16, 2023

 Currently Windows support is not available. This is primarily due to the difficulty in packaging grConvert’s dependencies for use in a Windows environment.

https://sjp.co.nz/projects/grconvert/

But I only have access to Windows machines... so I can't recreate the data. If you could find time to either attempt to open the existing data in RStudio see if it works, or run again the data script, it would be helpful in helping me unblock and move to the next steps.

@rempsyc
Copy link
Author

rempsyc commented Sep 16, 2023

Ok it seems like the last change I made fixes all remaining warnings and notes. Locally, I get:

── R CMD check results ─────────────────────────────────────────────────────────── ggflags 0.0.3 ────
Duration: 26.6s

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

R CMD check succeeded

Tested with my package also and it works. If you could merge that, I think it would be a great start.

@jimjam-slam
Copy link
Owner

Amazing! I'll look at merging this in tonight and adding ggflags to my R-Universe.

I have a Mac, so I can potentially install grConvert (which is probably what we need for #17).

@jimjam-slam jimjam-slam changed the base branch from master to dev September 18, 2023 08:04
@jimjam-slam
Copy link
Owner

Head up that I'm going to use this chance to set up R-Universe and also change the defualt branch from master to main, so I'll:

  1. Merge in to dev branch;
  2. Add ggflags to https://jimjam-slam.r-universe.dev using the dev branch;
  3. Rename the master branch to main and merge dev into that;
  4. Redirect https://jimjam-slam.r-universe.dev to use the main branch

@jimjam-slam
Copy link
Owner

Also @rempsyc, I'd love to add you to the package authors! Is that okay? Do you have an email address and/or ORC-ID you'd like me to list?

@jimjam-slam jimjam-slam merged commit 328d4bf into jimjam-slam:dev Sep 18, 2023
@rempsyc rempsyc deleted the document_flags branch September 18, 2023 12:46
@rempsyc
Copy link
Author

rempsyc commented Sep 18, 2023

Thanks, I appreciate it! You can use my info from here: https://github.com/easystats/effectsize/blob/main/DESCRIPTION

@rempsyc
Copy link
Author

rempsyc commented Sep 24, 2023

Head up that I'm going to use this chance to set up R-Universe and also change the defualt branch from master to main, so I'll:

  1. Merge in to dev branch;
  2. Add ggflags to https://jimjam-slam.r-universe.dev/ using the dev branch;
  3. Rename the master branch to main and merge dev into that;
  4. Redirect https://jimjam-slam.r-universe.dev/ to use the main branch

Good idea, can't wait to be able to install the new version on the main branch so my own R CMD check stops failing :D

@jimjam-slam
Copy link
Owner

So I'm running the dev branch at the moment, and it does passes all R CMD checks, but the demo example no longer actually plots flags anymore for me:

devtools::load_all()
# ℹ Loading ggflags
set.seed(1234)
d <- data.frame(x=rnorm(50), y=rnorm(50), 
                country=sample(c("ar","fr", "nz", "gb", "es", "ca", "lv", "qa"), 50, TRUE), 
                stringsAsFactors = FALSE)

ggplot(d, aes(x=x, y=y, country=country, size=x)) + 
  geom_flag() + 
  scale_country() +
  scale_size(range = c(0, 15))

test

Is that the case for you as well, @rempsyc? .flaglist does still appear to contain flags, and the main branch continues to work as expected:

test2

@jimjam-slam
Copy link
Owner

It looks like makeContent.flag is no longer called when the plot is rendered`. I think there might be a problem with the method registration!

@jimjam-slam
Copy link
Owner

Have tried adding @export to makeContent.flag, as I believe you need to export S3 methods in order to register them, but no dice. I'll try to come back to this tonight when I have a bit more time, and I'll check roxygen2 to confirm method registration requirements (especially as we're several major versions of {rOxygen2} down the line from this answer.

@jimjam-slam
Copy link
Owner

Okay, we're in business now! Added the @exportS3Method grid::makeContent tag to makeContent.flag and removed the line .flaglist <- utils::data("flags") (because .flaglist is automatically loaded from lflags.rda when the package is loaded, and if the package wasn't loaded you would attach the object .flaglist with just utils::data("lflags")).

@jimjam-slam
Copy link
Owner

Confirmed dev branch is now rendering correctly with no errors, warnings or notes on check!

@rempsyc
Copy link
Author

rempsyc commented Sep 29, 2023

Wouhouu, congrats and thanks!! Great work

@jimjam-slam
Copy link
Owner

Thanks again for all your work here!

@rempsyc
Copy link
Author

rempsyc commented Sep 30, 2023

Ok I just tried it with my package with the latest version. The good news is that it seems to have fixed my R CMD check.

However, it seems the line .flaglist <- utils::data("flags") was necessary, because when I try to run the function within my package through namespacing, and without loading your package, I get the following error:

Error in h(simpleError(msg, call)) : 
  error in evaluating the argument 'object' in selecting a method for function 'grobify': object '.flaglist' not found

Even trying what you suggest and locally calling the flags data within my function, I get:

packageVersion("ggflags")
#> [1] '0.0.3'
.flaglist <- utils::data("lflags")
#> Warning in utils::data("lflags"): data set 'lflags' not found
.flaglist <- ggflags::lflags
#> Error: 'lflags' is not an exported object from 'namespace:ggflags'

Created on 2023-09-30 with reprex v2.0.2

Can you try loading the flags data without having your package loaded see if it works on your end?

Seems like we're going to need another solution to make the function usable through namespacing... 🤔

@rempsyc
Copy link
Author

rempsyc commented Sep 30, 2023

Usually, the way to tackle this, as we discussed previously, is through usethis::use_data(): https://usethis.r-lib.org/reference/use_data.html

So I just tried, like this:

library(ggflags)
usethis::use_data("flaglist")

And it seems to have done something... it created a new dataset called .flaglist. By the way, the difference in names is highly confusing: the dataset is called lflags but refers to an underlying object called .flaglist. I think part of our problems comes from that. It seems like it may also create problems to have a data set name starting with a dot, so ideally, we would change the underlying object name to something else so that we don't have to deal with this problem.

@rempsyc
Copy link
Author

rempsyc commented Sep 30, 2023

Ok I think I found a way to fix it 🤞

Demo still works and R CMD check passes...

I realized that when loading the package and then using .flaglist, it did not add anything to my environment, and so I was not able to use usethis::use_data on the resulting data object. But what I failed to realize is that data sets starting with a period are actually HIDDEN data sets... I realized this through the R CMD check who mentioned something about us having a hidden data set when I tried to name them the same.

So what I did is that I created a new object, unhidden, through the old data set, like this (after loading the package):

lflags <- .flaglist

And this made the lflags object finally appear and I was finally able to see what was inside!!!!! This allowed me to use:

usethis::use_data(lflags)

Which created an UNHIDDEN data set called lflags, which could be properly documented, and also allowed me to delete the old .flaglist data set. After this, I am able to access it through ggflags::lflags, so that simplifies a lot of things... and data(lflags) now add it correctly to the environment.

Will try to submit a new PR with the changes...

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.

Several errors when attempting to integrate with other packages
2 participants