search

kite-sdk / kite

Kite SDK
http://kitesdk.org/docs/current/
Apache License 2.0
394 stars 263 forks source link

KITE-1091: Added a moveToTrash method to View, Dataset, Datasets, and… #424

Closed mkwhitacre closed 8 years ago

mkwhitacre commented 8 years ago

… DatasetRepository to support safer removal of data. Also converted the Kite DeleteCommand to default to using trash.

mkwhitacre commented 8 years ago

This will probably have conflict merges with #420 but this has bigger changes so I'll fix up #420 after this gets merged.

mkwhitacre commented 8 years ago

@rdblue any advice on how to troubleshoot the Travis build failures? Running the same commands locally and for each profile works so wasn't sure if you had any suggestions on how to more easily troubleshoot or simulate the builds locally.

rdblue commented 8 years ago

Sometimes failures that show up in Travis like this are due to running the tests in a different order. Make sure you don't have any dependencies between the tests that would be changed by the order in which they run. Using a TemporaryFolder rule to generate paths is usually a good option to ensure you don't have overlapping datasets.

mkwhitacre commented 8 years ago

Yeah I didn't really add new tests in that regard but just added new tests that were the existing delete tests to use the "move to trash". So might have exposed some ordering problems.

mkwhitacre commented 8 years ago

@rdblue got tests fixed by enabling trash for all tests (inconsistent Configuration objects used across tests). This should be good to review and critique.

rbrush commented 8 years ago

+1; looks good to me. Any objections to merging this?

Edit: disregard this for now; this comment was for another bug and I'm still looking at this one.

rbrush commented 8 years ago

Looks pretty good to me. Just a couple comments/questions above.

mkwhitacre commented 8 years ago

@rbrush thanks for the review The last commit should address your questions.

If you can give a quick look I'll then squash and rebase to make this merge-able.

mkwhitacre commented 8 years ago

Rebased the entire PR to make it merge-able.

rbrush commented 8 years ago

Looks like there is an extraneous println in assertDirectoriesInTrash, but +1 otherwise.

mkwhitacre commented 8 years ago

Ugh, thanks @rbrush it has been removed.

rbrush commented 8 years ago

+1. (The build failures appear to be unrelated to this change.)

mkwhitacre commented 8 years ago

@noslowerdna I removed the recursive trash and went with the delete. I did also rebase to a single commit so it is a cleaner merge.

noslowerdna commented 8 years ago

+1

mkwhitacre commented 8 years ago

Thanks @noslowerdna merging.