Refactor purge function #837

josenavas · 2015-02-09T21:25:11Z

Fixes (partially) #834

Some tables where missing on the purge_filepaths function. Now these tables are fetched programmatically, so this issue should not arise again.

Also, there was a line that should not be there at all, so I removed it. I'm actually surprise that the code was working with that line in there. The function move_filepaths_to_upload_folder also has that line added, so I removed it too.

…unction

coveralls · 2015-02-09T21:42:39Z

Coverage remained the same at 78.44% when pulling 26bb72d on josenavas:refactor-purge-function into 49e2e49 on biocore:master.

antgonza · 2015-02-10T12:04:58Z

Should we simply remove purge_filepaths? it's only used in a function that is not used any more

josenavas · 2015-02-10T16:01:27Z

It can be useful for #814

ElDeveloper · 2015-02-10T17:56:57Z

As long as it is correctly working and being used somewhere else, then
I'm all in for keeping it, otherwise there's no point.

On (Feb-10-15| 4:04), Antonio Gonzalez wrote:

Should we simply remove purge_filepaths? it's only used in a function that is not used any more

Reply to this email directly or view it on GitHub:
#837 (comment)

squirrelo · 2015-02-10T22:55:55Z

qiita_db/util.py

-            SELECT sequence_filepath FROM qiita.reference UNION
-            SELECT taxonomy_filepath FROM qiita.reference UNION
-            SELECT tree_filepath FROM qiita.reference)""")
+        WHERE filepath_id NOT IN (%s)""" % union_str)


Should this also make sure the files are legitimately deleted off the filesystem at the time of database removal?

Lines 724-739

Ah, sorry, not part of request so didn't see it. Carry on.

antgonza · 2015-02-12T15:21:15Z

Pull from upstream master.

coveralls · 2015-02-12T16:11:00Z

Coverage increased (+0.17%) to 78.61% when pulling f557296 on josenavas:refactor-purge-function into 49e2e49 on biocore:master.

antgonza · 2015-02-13T13:53:51Z

Note that @josenavas and I are checking why these changes do not work in my machine, we have some ideas but we are not sure yet. So please do not merge.

…unction

josenavas · 2015-02-13T16:33:04Z

@antgonza this is ready for review. I fixed the issue.

For other @biocore/qiita-dev, the issue is that when you're using the SQL operator NOT IN, if there is a NULL value on the list to test, the NOT IN will also return a NULL. For more information about the issue, check this link (thanks @antgonza for the useful link!)

antgonza · 2015-02-13T16:50:50Z

This now _actually_ works well, thanks for fixing. 👍

coveralls · 2015-02-13T16:51:42Z

Coverage remained the same at 78.5% when pulling 4ca81fd on josenavas:refactor-purge-function into 559556e on biocore:master.

josenavas · 2015-02-13T16:53:03Z

Thanks @antgonza !! Another reviewer?

ElDeveloper · 2015-02-13T20:44:03Z

qiita_db/test/test_util.py

@@ -421,6 +422,68 @@ def test_purge_filepaths(self):
        self.assertTrue(exists(fp1))
        self.assertFalse(exists(fp_exp_id))

+    def test_purge_filepaths_null_cols(self):
+        """Tests the nulls vs not in issue in purge_filepaths"""


What does this docstring mean? ...

There is an issue using the SQL command NOT IN if the list contains null values. There is a link in line 428 with more information. It was for clarity, but I think I did not achieve that; so I'm going to remove the docstring and leave only the comment with the link

Ahhh, now I understand the phrasing of the docstring. Thanks!

On (Feb-13-15|13:07), josenavas wrote:

@@ -421,6 +422,68 @@ def test_purge_filepaths(self):
self.assertTrue(exists(fp1))
self.assertFalse(exists(fp_exp_id))

def test_purge_filepaths_null_cols(self):

"""Tests the nulls vs not in issue in purge_filepaths"""

There is an issue using the SQL command NOT IN if the list contains null values. There is a link in line 428 with more information. It was for clarity, but I think I did not achieve that; so I'm going to remove the docstring and leave only the comment with the link

Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/837/files#r24696798

ElDeveloper · 2015-02-13T20:53:04Z

A few comments, looks in good shape. The one thing that struck my attention was how complex the test method is, I would advise breaking it down into individual cases, but there's no reason why this should be a blocking comment.

josenavas · 2015-02-13T21:23:50Z

The method is quite complex, but is not testing multiple things. It is only testing one execution of the purge_filepaths function. Since it is a quite dangerous functionality, I decided to put quite few multiple checks and things that I think that functionality could potentially mess up

coveralls · 2015-02-13T21:43:24Z

Coverage decreased (-0.02%) to 78.48% when pulling 3a58a90 on josenavas:refactor-purge-function into 559556e on biocore:master.

…unction

josenavas · 2015-02-18T01:30:31Z

This should be ready for re-re-re....-review! I've simplified tests and written a new function. It turns out that we already have unlinked files on the test database so I did not need to add a new unlinked file. Also, there was a bug on the fp removal, which is interesting since it was working before...

coveralls · 2015-02-18T01:45:44Z

Coverage increased (+0.01%) to 78.36% when pulling 155a9d3 on josenavas:refactor-purge-function into d398414 on biocore:master.

ElDeveloper · 2015-02-18T05:52:14Z

This looks good, thanks for making these changes @josenavas 👍

On (Feb-17-15|17:45), Coveralls wrote:

Coverage increased (+0.01%) to 78.36% when pulling 155a9d3 on josenavas:refactor-purge-function into d398414 on biocore:master.

Reply to this email directly or view it on GitHub:
#837 (comment)

antgonza · 2015-02-18T14:35:49Z

qiita_db/util.py

@@ -612,6 +612,28 @@ def get_mountpoint(mount_type, conn_handler=None, retrieve_all=False):
    return [(d, join(basedir, m, s)) for d, m, s in result]


+def get_mountpoint_path(mount_id, conn_handler=None):


This can be confused with get_mountpoint, perhaps change to get_mountpoint_path_by_id?

ElDeveloper · 2015-02-18T19:28:37Z

@josenavas, it looks like there are some flake8 problems, may be related to a newer version of flake8. Not your code, it's from the codebase in general.

josenavas · 2015-02-18T19:29:37Z

Yes, I just saw them. I will try to fix them here, as the PR is ready to go...

josenavas · 2015-02-18T19:33:42Z

@ElDeveloper @antgonza the issues due to the new version of pep8 should have been solved with my last commit.

coveralls · 2015-02-18T20:00:27Z

Coverage increased (+0.03%) to 78.37% when pulling dba6d64 on josenavas:refactor-purge-function into d398414 on biocore:master.

josenavas · 2015-02-18T20:01:06Z

@ElDeveloper @antgonza any remaining comment?

Refactor purge function

josenavas added 3 commits February 9, 2015 13:07

Refactoring purge_filepaths

e16168e

Merge branch 'master' of github.com:qiime/QiiTa into refactor-purge-f…

b10d40e

…unction

Fixing move_filepaths_to_upload_folder

26bb72d

squirrelo reviewed Feb 10, 2015
View reviewed changes

Changing SO link to permalink

f557296

josenavas added 2 commits February 13, 2015 08:03

Merge branch 'master' of github.com:qiime/QiiTa into refactor-purge-f…

1095cc9

…unction

solving null vs not in issue

4ca81fd

ElDeveloper reviewed Feb 13, 2015
View reviewed changes

Addressing @ElDeveloper comments

3a58a90

josenavas added 2 commits February 17, 2015 11:57

Merge branch 'master' of github.com:qiime/QiiTa into refactor-purge-f…

66bc2e4

…unction

Adding get_mountpoint_path function

6fd04c5

josenavas added 2 commits February 17, 2015 14:40

Addressing comments

9b230a2

flake8...

155a9d3

antgonza reviewed Feb 18, 2015
View reviewed changes

josenavas added 2 commits February 18, 2015 09:44

Addressing @antgonza comments

002c3a9

flake8

373f7ff

Hating new versions of pep8

dba6d64

josenavas mentioned this pull request Feb 18, 2015

Flake8 requirements have changed (again) #864

Closed

antgonza added a commit that referenced this pull request Feb 18, 2015

Merge pull request #837 from josenavas/refactor-purge-function

557a7dd

Refactor purge function

antgonza merged commit 557a7dd into qiita-spots:master Feb 18, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor purge function #837

Refactor purge function #837

josenavas commented Feb 9, 2015

coveralls commented Feb 9, 2015

antgonza commented Feb 10, 2015

josenavas commented Feb 10, 2015

ElDeveloper commented Feb 10, 2015

squirrelo Feb 10, 2015

josenavas Feb 10, 2015

squirrelo Feb 10, 2015

antgonza commented Feb 12, 2015

coveralls commented Feb 12, 2015

antgonza commented Feb 13, 2015

josenavas commented Feb 13, 2015

antgonza commented Feb 13, 2015 via email

coveralls commented Feb 13, 2015

josenavas commented Feb 13, 2015

ElDeveloper Feb 13, 2015

josenavas Feb 13, 2015

ElDeveloper Feb 13, 2015

ElDeveloper commented Feb 13, 2015

josenavas commented Feb 13, 2015

coveralls commented Feb 13, 2015

josenavas commented Feb 18, 2015

coveralls commented Feb 18, 2015

ElDeveloper commented Feb 18, 2015

antgonza Feb 18, 2015

josenavas Feb 18, 2015

ElDeveloper commented Feb 18, 2015

josenavas commented Feb 18, 2015

josenavas commented Feb 18, 2015

coveralls commented Feb 18, 2015

josenavas commented Feb 18, 2015

		@@ -612,6 +612,28 @@ def get_mountpoint(mount_type, conn_handler=None, retrieve_all=False):
		return [(d, join(basedir, m, s)) for d, m, s in result]


		def get_mountpoint_path(mount_id, conn_handler=None):

Refactor purge function #837

Refactor purge function #837

Conversation

josenavas commented Feb 9, 2015

coveralls commented Feb 9, 2015

antgonza commented Feb 10, 2015

josenavas commented Feb 10, 2015

ElDeveloper commented Feb 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgonza commented Feb 12, 2015

coveralls commented Feb 12, 2015

antgonza commented Feb 13, 2015

josenavas commented Feb 13, 2015

antgonza commented Feb 13, 2015 via email

coveralls commented Feb 13, 2015

josenavas commented Feb 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElDeveloper commented Feb 13, 2015

josenavas commented Feb 13, 2015

coveralls commented Feb 13, 2015

josenavas commented Feb 18, 2015

coveralls commented Feb 18, 2015

ElDeveloper commented Feb 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElDeveloper commented Feb 18, 2015

josenavas commented Feb 18, 2015

josenavas commented Feb 18, 2015

coveralls commented Feb 18, 2015

josenavas commented Feb 18, 2015