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

Do not load sysdata.rda #347

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Do not load sysdata.rda #347

merged 1 commit into from
Oct 2, 2020

Conversation

jozefhajnala
Copy link
Contributor

In the R directory, some packages have the sysdata.rda file (see https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Package-subdirectories for reference), which tries to get loaded on_initialized. This PR proposes to skip that file from load_all().

@randy3k
Copy link
Member

randy3k commented Oct 1, 2020

a better solution may be to load only files end with .R or .r.

@jozefhajnala
Copy link
Contributor Author

a better solution may be to load only files end with .R or .r.

I wanted to err on the side of conservativism, as there could also be extensions .S, .q, or .s for source files and .in files for configure scripts, but of course if you are happy with just loading .R or .r I can update the PR.

@jozefhajnala
Copy link
Contributor Author

I updated the PR, if you would like to re-review.

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

@renkun-ken
Copy link
Member

@randy3k Do we need to resolve the CI issue before merging?

@randy3k
Copy link
Member

randy3k commented Oct 2, 2020

This one looks good to me.

@randy3k
Copy link
Member

randy3k commented Oct 2, 2020

there could also be extensions .S, .q, or .s for source files.

I don't think it makes sense to support all the ancient file types. Anyway, thank you for your contribution.

@randy3k randy3k merged commit 21d796b into REditorSupport:master Oct 2, 2020
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

3 participants