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

Cleanup old code #240

Open
3 tasks
Hugovdberg opened this issue Apr 28, 2018 · 7 comments
Open
3 tasks

Cleanup old code #240

Hugovdberg opened this issue Apr 28, 2018 · 7 comments
Labels

Comments

@Hugovdberg
Copy link
Collaborator

Hugovdberg commented Apr 28, 2018

While documenting all functions I discovered a few functions that seem to be unused anywhere. They are not called from other functions and are not exported into the namespace. I'm listing candidates for removal here, please comment to substantiate why we should keep them:

  • cache.name in R/cache.name.R
  • .cache.status in R/cache.R
  • .unload.project in R/load.project.R (This function is only called from two tests where they seem to test completely unrelated stuff. Those function calls should be removed anyway, if clearing memory is necessary clear should be preferred)

If you think more functions can be removed also please comment.

These functions should be marked deprecated by use of the .Deprecated function, so we later can mark them .Defunct and eventually remove them altogether.

@KentonWhite
Copy link
Owner

Agree to removing cache.name and .cache.status. Deprecating first sound like a good approach.

unload.project is used when testing the migration code. I'm not yet sure if we want to remove the migration testing. It served a purpose in the past (when we had some pretty major changes and needed to ensure that all migrations were done properly) and could be useful once again!

@rsangole
Copy link
Collaborator

rsangole commented Apr 30, 2018

Not a function, but there's a 4year+ old TODO document which seems out of date.

@KentonWhite
Copy link
Owner

I think it is safe to remove the TODO.markdown file since we are tracking issues now on GitHub!

@rsangole
Copy link
Collaborator

rsangole commented May 1, 2018

I had this in #210 , but I think it's more appropriate here.


When I was working on #234 , I found that create.project() has two calls to .stopifproject which seem redundant.

.stopifproject(c("Cannot create a new project inside an existing one",
"Please change to another directory and re-run create.project()"))
.stopifproject(c("Cannot create a new project inside an existing one",
"Please change to another directory and re-run create.project()"),
path = dirname(getwd()))

The 1st call works fine and stops correctly. If the 1st call is commented out, the 2nd call does not work correctly and creates a project within a project.

@KentonWhite
Copy link
Owner

Nice spot @rsangole. Lets move the fix here as you suggest.

@Hugovdberg
Copy link
Collaborator Author

Hmm I'm actually thinking that is just a separate bug, I was working on a fix but haven't created the PR yet. It really are two separate checks which both are useful and can't be merged. They just check the wrong directory.

@KentonWhite
Copy link
Owner

ok I'll reopen #210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants