Skip to content

[WIP] Add table metadata to cmiyc#118

Open
khalidharun wants to merge 18 commits intorobertzk:masterfrom
khalidharun:table_metadata
Open

[WIP] Add table metadata to cmiyc#118
khalidharun wants to merge 18 commits intorobertzk:masterfrom
khalidharun:table_metadata

Conversation

@khalidharun
Copy link
Copy Markdown
Collaborator

In order to shed some more light into what the table_name refers back to, let's start storing some table metadata which will store the salt values.

Comment thread R/db.R Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringsAsFactors = FALSE

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> serialize("foo")
Error in serialize("foo") :
  argument "connection" is missing, with no default

don't you need NULL as the second argument to serialize?

@robertzk
Copy link
Copy Markdown
Owner

robertzk commented Dec 7, 2015

Few minor comments. Can you get tests passing?

Comment thread R/metadata.R Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this a separate function.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure_cache_metadata_table_exists()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@robertzk
Copy link
Copy Markdown
Owner

robertzk commented Dec 8, 2015

Some minor comments but otherwise good.

@peterhurford
Copy link
Copy Markdown
Collaborator

@khalidharun @robertzk Ping?

Comment thread R/db.R Outdated

get_table_name <- memoise::memoise(table_name_)

#' TODO: Depricate in favor of get_table_name
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the roxygen in this comment?

@robertzk
Copy link
Copy Markdown
Owner

@khalidharun Can you address comments>

@robertzk
Copy link
Copy Markdown
Owner

Will merge after addressed.

@peterhurford
Copy link
Copy Markdown
Collaborator

Definitely should backmerge master now that #115 is merged

@robertzk
Copy link
Copy Markdown
Owner

@khalidharun can you do so? Don't have control from over here.

@robertzk
Copy link
Copy Markdown
Owner

@khalidharun welcome to the inner circle.

screen shot 2016-03-28 at 9 47 42 pm

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.

4 participants