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 use dir.exists to retain backwards-compatibility #698

Merged
merged 1 commit into from May 25, 2017

Conversation

Projects
None yet
4 participants
@epipping
Contributor

epipping commented May 24, 2017

The file R/Attributes.R uses the function dir.exists() which
was only added in R 3.2.0. The relevant line was added to
R/Attributes.R between 0.12.5 and 0.12.6.

@eddelbuettel

Good catch.

@epipping

This comment has been minimized.

Show comment
Hide comment
@epipping

epipping May 24, 2017

Contributor

If compatibility with versions older than 3.2 is important (debian 8.8 still only has 3.1) one might want to turn the dir.exists back into file.exists instead.

Contributor

epipping commented May 24, 2017

If compatibility with versions older than 3.2 is important (debian 8.8 still only has 3.1) one might want to turn the dir.exists back into file.exists instead.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 24, 2017

Member

Debian 8.8 is irrelevant as its sources for Rcpp match its sources for base R. Plus, we have the actively supported backport for R available via CRAN.

We could use the backports package, or at least Suggests: it if we cared.

Team: Anybody see a problem depending on the now-two-year old R 3.2.0 ?

Member

eddelbuettel commented May 24, 2017

Debian 8.8 is irrelevant as its sources for Rcpp match its sources for base R. Plus, we have the actively supported backport for R available via CRAN.

We could use the backports package, or at least Suggests: it if we cared.

Team: Anybody see a problem depending on the now-two-year old R 3.2.0 ?

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey May 24, 2017

Contributor

I'm guessing there are a lot of users in enterprise environments who are stuck with R 3.1.x or even R 3.0.x who would appreciate if we could remain compatible with R 3.0.0 and above, so I would be marginally in favor of just using file.exists() or utils::file_test("-d", ...) to be backwards compatible -- especially since the change is so small in this case.

Contributor

kevinushey commented May 24, 2017

I'm guessing there are a lot of users in enterprise environments who are stuck with R 3.1.x or even R 3.0.x who would appreciate if we could remain compatible with R 3.0.0 and above, so I would be marginally in favor of just using file.exists() or utils::file_test("-d", ...) to be backwards compatible -- especially since the change is so small in this case.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 24, 2017

Member

Yes, that is a good policy. I can look into that while traveling the next two days. Of course I would be equally happy to receive a short PR :)

Member

eddelbuettel commented May 24, 2017

Yes, that is a good policy. I can look into that while traveling the next two days. Of course I would be equally happy to receive a short PR :)

Do not use dir.exists for better compatibility
The function dir.exists was only added in R 3.2

@epipping epipping changed the title from Declare dependency: R >= 3.2.0 to Do not use dir.exists to retain backwards-compatibility May 24, 2017

@epipping

This comment has been minimized.

Show comment
Hide comment
@epipping

epipping May 24, 2017

Contributor

utils::file_test("-d", ...) is the perfect solution for this problem. Since I see no disadvantages in following that route, for simplicity, I've now hijacked my own pull request and turned it into this different solution for the same problem by force-pushing.

Contributor

epipping commented May 24, 2017

utils::file_test("-d", ...) is the perfect solution for this problem. Since I see no disadvantages in following that route, for simplicity, I've now hijacked my own pull request and turned it into this different solution for the same problem by force-pushing.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire May 24, 2017

Member

Yes, we have to remember that we bump our required version we implicitly bump the required version of 92% of CRAN (via recursive dependencies).

Member

jjallaire commented May 24, 2017

Yes, we have to remember that we bump our required version we implicitly bump the required version of 92% of CRAN (via recursive dependencies).

@eddelbuettel eddelbuettel merged commit d6ec27f into RcppCore:master May 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment