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

Fix s3data export everyone talks about but no one fixes it #29

Conversation

FeiYeYe
Copy link
Collaborator

@FeiYeYe FeiYeYe commented Mar 19, 2015

the take away is environment object (aka tundra container) is not an environment and names on it will be null so have to carefully check if object$output$options$data is in, that's why we need bring back name_exists

@@ -232,17 +232,15 @@ construct_s3_adapter <- function() {
write_function <- function(object, opts) {
common_s3mpi_package_loader()

if (is.element('output', names(object))) {
Copy link
Owner

Choose a reason for hiding this comment

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

Just replace names(object) with ls(object) -- ls works for lists and environments, although it does not preserve order (which we don't care about here).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ls is not recursive and might fail if one if the object in the middle does not exist
and this kind of failture seems to be silent when running syberia (still not sure why)..

Copy link
Collaborator

Choose a reason for hiding this comment

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

💥 my mind is blown

@robertzk robertzk closed this Mar 19, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f4707da on fix_s3data_export_everyone_talks_about_but_no_one_fixes_it into * on master*.

@peterhurford
Copy link
Collaborator

Closed with unmerged commits?

@robertzk
Copy link
Owner

@peterhurford we went with a different approach (s/names/ls)

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

4 participants