`asearch` function usage #149

wchodor · 2015-11-30T13:20:09Z

@pbiecek
I am in the course of reviewing asearch function and I'm wondering what was your goal. Were you planning to extend its functionality on local repository? This part of asearch function's body indicates that we want to search and read from local repo too. But it can be only default local repo.

if (is.null(repo)) {
    # use default repo
    oblist <- multiSearchInLocalRepo(patterns = patterns,
                                      intersect = TRUE)
    if (length(oblist)>0) {
      res <- lapply(oblist, aread)
    } 
  }

I am a little bit confused because the documentation suggests that we want to work on Github Repository only.

The text was updated successfully, but these errors were encountered:

MarcinKosinski · 2015-11-30T14:53:01Z

I think it's just the matter of obsolete documentation.

pbiecek · 2015-11-30T14:57:36Z

The documentation is to be updated.
My goal is to cover to options:
If there is any '/' then it search in github repo,
If there is no / then it search in the default repo, either local or github.

P

Dnia 30.11.2015 o godz. 14:20 Witold Chodor notifications@github.com napisał(a):

@pbiecek
I am in the course of reviewing asearch function and I'm wondering what was your goal. Were you planning to extend its functionality on local repository? This part of asearch function's body indicates that we want to search and read from local repo too. But it can be only default local repo.

if (is.null(repo)) {
# use default repo
oblist <- multiSearchInLocalRepo(patterns = patterns,
intersect = TRUE)
if (length(oblist)>0) {
res <- lapply(oblist, aread)
}
}
I am a little bit confused because the documentation suggests that we want to work on Guthub Repository only.

—
Reply to this email directly or view it on GitHub.

wchodor · 2015-11-30T15:14:41Z

OK, I understand.
But the actual code won't work for default Github repository. I will try to fix it.

pbiecek · 2015-11-30T16:11:35Z

Would be great if you can take care of it.

2015-11-30 16:14 GMT+01:00 Witold Chodor notifications@github.com:

So tha actual code won't work for Github default repository. Do you want
me to take care of it?

—
Reply to this email directly or view it on GitHub
#149 (comment).

pozdrawiam serdecznie,
Przemysław Biecek

wchodor · 2015-12-01T16:56:20Z

@pbiecek
I made some changes and now it is working. I would appreciate if you tell me whether such changes meet you expectations. Here is the link to commit:
https://github.com/wchodor/archivist/commit/b7bef3279b8e8ba6fb0a85c43f1e7015b2f41811

wchodor · 2015-12-01T17:04:23Z

@MarcinKosinski @pbiecek
What is more I suggest removing default parameter from createEmptyLocalRepo function.
First of all because of the fact that it is not used in createEmptyGithubRepo.
Secondly asearch won't work with default=TRUE argument in createEmptyLocalRepo.

MarcinKosinski · 2015-12-01T17:06:39Z

There's an issue for default parameter in createEmptyGithubRepo:
#143

Why asearch won't work with default=TRUE argument in createEmptyLocalRepo?

2015-12-01 18:04 GMT+01:00 Witold Chodor notifications@github.com:

@MarcinKosinski https://github.com/MarcinKosinski @pbiecek
https://github.com/pbiecek
What is more I suggest removing default parameter from
createEmptyLocalRepo function.
First of all because of the fact that it is not used in
createEmptyGithubRepo.
Secondly asearch won't work with default=TRUE argument in
createEmptyLocalRepo.

—
Reply to this email directly or view it on GitHub
#149 (comment).

wchodor · 2015-12-01T17:15:29Z

Because of setLocalRepo. I assumed that type parameter is loaded into .ArchivEnv in setRepo function. So when you are using createEmptyRepo with default=TRUE parameter then setLocalRepo is used and type is not loaded. As a result multiSearchInRepo in asearch gives an error.

MarcinKosinski · 2015-12-01T17:48:29Z

So maybe createEmptyRepo with default=TRUE should also change global type
parameter?

2015-12-01 18:15 GMT+01:00 Witold Chodor notifications@github.com:

Because of setLocalRepo. I assumed that type parameter is loaded into
.ArchivEnc in setRepo function. So when you are using createEmptyRepo
with default=TRUE parameter then type is not loaded. And multiSearchInRepo
in asearch gives an error.

—
Reply to this email directly or view it on GitHub
#149 (comment).

wchodor · 2015-12-01T17:53:33Z

Do you mean to put

aoptions( "type", type )

in createEmpty Repo' body?

wchodor · 2015-12-01T17:54:52Z

like this:

stopifnot(is.character(type) & length(type) ==1 & type %in% c("local", "github"))
if (default) 
    aoptions( "type", type )
  if (type == "local") {
    createEmptyLocalRepo(repoDir = repoDir, force = force, default = default)
  } else {

MarcinKosinski · 2015-12-01T17:56:19Z

if deafult == TRUE ?

2015-12-01 18:54 GMT+01:00 Witold Chodor notifications@github.com:

stopifnot(is.character(type) & length(type) ==1 & type %in% c("local", "github"))
aoptions( "type", type )
if (type == "local") {
createEmptyLocalRepo(repoDir = repoDir, force = force, default = default)
} else {

—
Reply to this email directly or view it on GitHub
#149 (comment).

wchodor · 2015-12-01T17:58:05Z

So, like this ?

  stopifnot(is.character(type) & length(type) ==1 & type %in% c("local", "github"))
  if (default) aoptions("type", type)
  if (type == "local") {
    createEmptyLocalRepo(repoDir = repoDir, force = force, default = default)
  } else {

MarcinKosinski · 2015-12-01T18:01:14Z

Looks good. Pleas do the change and submit PR

2015-12-01 18:58 GMT+01:00 Witold Chodor notifications@github.com:

stopifnot(is.character(type) & length(type) ==1 & type %in% c("local", "github"))
if (default) aoptions("type", type)
if (type == "local") {
createEmptyLocalRepo(repoDir = repoDir, force = force, default = default)
} else {

—
Reply to this email directly or view it on GitHub
#149 (comment).

pbiecek · 2015-12-01T18:18:11Z

Why do we need 'type = aoptions("type")' ?
It looks like a duplication, since the loadFromRepo() function can decide
if someone is asking for local repo (repoDir is set)
or github repo (repo is set).

it is actually a possible vulnerability since default value of type may not
be consistent with default value of repoDir/repo.

2015-12-01 17:56 GMT+01:00 Witold Chodor notifications@github.com:

@pbiecek https://github.com/pbiecek
I made some changes and now it is working. I would appreciate if you tell
me whether such changes meet you expectations. Here is the link to commit:
wchodor@b7bef32
https://github.com/wchodor/archivist/commit/b7bef3279b8e8ba6fb0a85c43f1e7015b2f41811

—
Reply to this email directly or view it on GitHub
#149 (comment).

pozdrawiam serdecznie,
Przemysław Biecek

pbiecek · 2015-12-01T18:21:00Z

Regarding the 'default' parameter (let's use separate issues for separate
problems),
I would rather fix asearch (to make it working with default local repo) and
createEmptyGithubRepo (which is still in development I believe)

2015-12-01 18:04 GMT+01:00 Witold Chodor notifications@github.com:

@MarcinKosinski https://github.com/MarcinKosinski @pbiecek
https://github.com/pbiecek
What is more I suggest removing default parameter from
createEmptyLocalRepo function.
First of all because of the fact that it is not used in
createEmptyGithubRepo.
Secondly asearch won't work with default=TRUE argument in
createEmptyLocalRepo.

—
Reply to this email directly or view it on GitHub
#149 (comment).

pozdrawiam serdecznie,
Przemysław Biecek

pbiecek · 2015-12-01T18:25:45Z

The fact that you need to synchronise changes in 'type' and 'repoDir' is an
argument against using type as a parameter.
Instead of that (if we really need type, I am not convinced) it may be a
function which will decide if the definition of a repository matches local
repo or github repo (it can be decided based of names of arguments).

2015-12-01 19:01 GMT+01:00 Marcin Kosiński notifications@github.com:

Looks good. Pleas do the change and submit PR

2015-12-01 18:58 GMT+01:00 Witold Chodor notifications@github.com:

stopifnot(is.character(type) & length(type) ==1 & type %in% c("local",
"github"))
if (default) aoptions("type", type)
if (type == "local") {
createEmptyLocalRepo(repoDir = repoDir, force = force, default = default)
} else {

—
Reply to this email directly or view it on GitHub
<#149 (comment)
.

—
Reply to this email directly or view it on GitHub
#149 (comment).

pozdrawiam serdecznie,
Przemysław Biecek

wchodor · 2015-12-01T18:46:41Z

I understand your point of view.
I will try to think how to fix this in the way you say.

wchodor · 2015-12-01T21:43:05Z

@pbiecek
I made some changes regarding your hints. Above, you have my commit.

Alterations in `asearch` and other functions due to #149 issue.

pbiecek · 2015-12-01T21:58:29Z

Thanks,
btw: maybe we can add to the wiki list of parameters, that can be set via 'aoptions'?
Then it will be easier to see parameters like 'type'

wchodor · 2015-12-01T22:16:18Z

When it comes to type I removed it and used repoDir and repo arguments to detect whether repository is local or GitHub.
As for now type parameter is only in createEmptyRepo. I thought that you suggested not to use it. And I think that it was a good idea. I proposed such a solution (e.g. for loadFromRepo):

loadFromRepo <- function( md5hash, repoDir = NULL,
                          repo = NULL, user = NULL, branch = "master", repoDirGit = FALSE,
                          value = FALSE ){

  local <- (!is.null(aoptions("repoDir")) && is.null(repo)) || (!is.null(repoDir) && is.null(repo))
  GitHub <- (is.null(repoDir) && !is.null(aoptions("repo"))) || (is.null(repoDir) && !is.null(repo))
  if (local) {
    loadFromLocalRepo( md5hash = md5hash, repoDir = repoDir, value = value )
  } else if (GitHub) {
    loadFromGithubRepo(  md5hash = md5hash, repo = repo, user = user, branch = branch,
                         repoDirGit = repoDirGit, value = value )
  } else {
    stop("repo and repoDir parameters can not be used simultaneously.")
  } 
}

MarcinKosinski · 2015-12-06T11:17:23Z

Ok looks good. If this is fixed, can you @wchodor close the issue?

wchodor mentioned this issue Nov 30, 2015

asearch wchodor/DataArtists#61

Closed

pbiecek added a commit that referenced this issue Dec 1, 2015

Merge pull request #158 from wchodor/master

4b4ced5

Alterations in `asearch` and other functions due to #149 issue.

wchodor closed this as completed Dec 7, 2015

This was referenced Dec 7, 2015

Omowienie wraperow wchodor/DataArtists#136

Closed

Create wrappers for all possible Github/local functions #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`asearch` function usage #149

`asearch` function usage #149

wchodor commented Nov 30, 2015

MarcinKosinski commented Nov 30, 2015

pbiecek commented Nov 30, 2015

wchodor commented Nov 30, 2015

pbiecek commented Nov 30, 2015

wchodor commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

wchodor commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

pbiecek commented Dec 1, 2015

pbiecek commented Dec 1, 2015

pbiecek commented Dec 1, 2015

wchodor commented Dec 1, 2015

wchodor commented Dec 1, 2015

pbiecek commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 6, 2015

asearch function usage #149

asearch function usage #149

Comments

wchodor commented Nov 30, 2015

MarcinKosinski commented Nov 30, 2015

pbiecek commented Nov 30, 2015

wchodor commented Nov 30, 2015

pbiecek commented Nov 30, 2015

wchodor commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

wchodor commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 1, 2015

pbiecek commented Dec 1, 2015

pbiecek commented Dec 1, 2015

pbiecek commented Dec 1, 2015

wchodor commented Dec 1, 2015

wchodor commented Dec 1, 2015

pbiecek commented Dec 1, 2015

wchodor commented Dec 1, 2015

MarcinKosinski commented Dec 6, 2015

`asearch` function usage #149

`asearch` function usage #149