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

bump to v0.3.14 #578

Merged
merged 6 commits into from
Oct 16, 2022
Merged

bump to v0.3.14 #578

merged 6 commits into from
Oct 16, 2022

Conversation

randy3k
Copy link
Member

@randy3k randy3k commented Oct 13, 2022

Will merge after CRAN accepts the submission.

@randy3k
Copy link
Member Author

randy3k commented Oct 15, 2022

@renkun-ken CRAN check doesn't like the use of attach, PTAL.

R/diagnostics.R Outdated
attach(globals, name = env_name, warn.conflicts = FALSE)
on.exit(detach(env_name, character.only = TRUE))
parent.env(globals) <- environment()
lints <- with(globals, lintr::lint(path, cache = cache, text = content))
Copy link
Member

Choose a reason for hiding this comment

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

What if globals contains symbols such as path, and cache, and content?

Copy link
Member

Choose a reason for hiding this comment

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

I tried putting path <- NULL in a package R file, and the following occurs:

Failed to run diagnostics: ! error in callr subprocess
Caused by error in `if (needs_tempfile) { ...`:
! missing value where TRUE/FALSE needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Let me fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the variables must be accessible from the global environment. Looks like globals must be attached and we'll have to walk-around the CRAN policy?

Copy link
Member

Choose a reason for hiding this comment

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

Just like what callr has done using do.call("attach", ...) at

https://github.com/r-lib/callr/blob/5adbb27027c377b3ad0d4b3b471f81b36b02da67/R/hook.R#L7

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Already fixed via 8b27a18.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, submitting to CRAN.

Copy link
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

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

LGTM

@randy3k
Copy link
Member Author

randy3k commented Oct 16, 2022

It's on the way to CRAN.

@randy3k randy3k merged commit a939390 into master Oct 16, 2022
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.

None yet

2 participants