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

Now CreateLocalRepo always returns repo path, even when Default=FALSE #291

Merged
merged 1 commit into from
Aug 2, 2016

Conversation

adamryczkowski
Copy link
Contributor

All examples fail when CreateLocalRepo isn't invoked with default=TRUE parameter, because when default=FALSE the function returns NULL.

Why bother with the return value? - Because:

  1. The CreateLocalRepo works very much like object factory, and a lot of people would assume it returns something more complex, than a string.
  2. Indeed, sooner or later you might want to return something that would speed object lookup, like the SQL connection object itself or something even more elaborate.

@pbiecek
Copy link
Owner

pbiecek commented Aug 2, 2016

Good idea,

In fact in the beginning repositories were represented as objects. But with time it turns out that they contain only a single string thus the repo was reduced to a single string.

However now, when we need to handle URL repos, local repos, Git repos and maybe some more advanced storages in future, it is tempting to add an S4/R5 interface with functions like

repo$save()
repo$search()
repo$delete()
repo$read()
repo$summary()

@MarcinKosinski what do you think? Maybe creation of such interface could be a project for students [i.e. within R and Big Data]?

@pbiecek pbiecek merged commit 40f441a into pbiecek:master Aug 2, 2016
@MarcinKosinski
Copy link
Collaborator

@pbiecek I think we wrote about S4 classes for repositories here #153 (comment)

@adamryczkowski could you please extend NEWS.md file with your change and provide another PR? Thanks for effort :)

@pbiecek
Copy link
Owner

pbiecek commented Aug 2, 2016

@MarcinKosinski Right, I forgot about it, but it is a great summary for future mappings, thanks for linking it.

Tempting idea: submit proposal to R Consortium to build ArchivistHub, large repository of R (and not only) binary objects. This might be a good excuse for new interface for archivist.

@adamryczkowski
Copy link
Contributor Author

I love R6, and if I were you, I'd consider using this instead of S4 or Reference classes.

adamryczkowski added a commit to adamryczkowski/archivist that referenced this pull request Aug 8, 2016
@MarcinKosinski
Copy link
Collaborator

Isn't R6's typeof a S4?

By the way, how could we reach R Comsorcium with this idea?

Marcin Kosinski

Dnia 08.08.2016 o godz. 09:06 Adam Ryczkowski notifications@github.com napisał(a):

I love R6, and if I were you, I'd consider using this instead of S4 or Reference classes.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@pbiecek
Copy link
Owner

pbiecek commented Aug 8, 2016

Need to wait for next call for proposals
https://www.r-consortium.org/projects/call-proposals

R6 is different than S4,
Here is a nice description
https://cran.r-project.org/web/packages/R6/vignettes/Introduction.html

Dnia 08.08.2016 o godz. 11:40 Marcin Kosiński notifications@github.com napisał(a):

Isn't R6's typeof a S4?

By the way, how could we reach R Comsorcium with this idea?

Marcin Kosinski

Dnia 08.08.2016 o godz. 09:06 Adam Ryczkowski notifications@github.com napisał(a):

I love R6, and if I were you, I'd consider using this instead of S4 or Reference classes.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@MarcinKosinski
Copy link
Collaborator

archivist.R6 :) ?

2016-08-08 14:02 GMT+02:00 Przemysław Biecek notifications@github.com:

Need to wait for next call for proposals
https://www.r-consortium.org/projects/call-proposals

R6 is different than S4,
Here is a nice description
https://cran.r-project.org/web/packages/R6/vignettes/Introduction.html

Dnia 08.08.2016 o godz. 11:40 Marcin Kosiński notifications@github.com
napisał(a):

Isn't R6's typeof a S4?

By the way, how could we reach R Comsorcium with this idea?

Marcin Kosinski

Dnia 08.08.2016 o godz. 09:06 Adam Ryczkowski notifications@github.com
napisał(a):

I love R6, and if I were you, I'd consider using this instead of S4 or
Reference classes.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#291 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGdazopGCZw-vvdNQW6EO76gkk9-kOxhks5qdxrLgaJpZM4Ja2Sa
.

@MarcinKosinski
Copy link
Collaborator

Getting back to this PR.
How this commit has changed our examples?

Examples worked because they had default=TRUE parameter set in createEmptyRepo.
Returning repoDir after createEmptyRepo call does not call setLocalRepo and in my opinion if you don't use defualt=TRUE in createEmptyRepoand you don't callsetLocalRepo`, then examples will not work still.

But our examples has defualt=TRUE in createEmptyRepo so I still don't see the point.

MarcinKosinski added a commit that referenced this pull request Aug 9, 2016
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