Issue 835 #845

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

This fixes the analysis filepath issues and adds a test to make sure it doesn't happen again.

ElDeveloper · 2015-02-10T22:49:14Z

@squirrelo, there are legitimate flake8 errors in this build.

squirrelo · 2015-02-10T22:51:40Z

Not any more.

ElDeveloper · 2015-02-10T22:52:21Z

💥 💥

coveralls · 2015-02-10T23:09:53Z

Coverage increased (+0.0%) to 78.61% when pulling ab3ac5c on squirrelo:issue-835 into 32d6d8e on biocore:master.

ElDeveloper · 2015-02-11T00:13:59Z

qiita_db/test/test_analysis.py

@@ -427,6 +428,26 @@ def test_build_files_raises_value_error(self):
        with self.assertRaises(ValueError):
            self.analysis.build_files(-10)

+    def test_add_file(self):
+        fp = join(get_mountpoint('analysis')[0][1], 'testfile.txt')
+        try:


If I understand correctly: if you can write to the file path you add it to the analysis and regardless of the result of these two lines you check if it exists and if it exists you remove it? Should that fp be removed in the teardown method of the test class? So we can guarantee that it is removed, the finally block here is kinda confusing.

ElDeveloper · 2015-02-11T00:14:46Z

This PR looks ok but I'm not super familiar with this codebase so it would be nice if someone else could review it in depth.

squirrelo · 2015-02-11T04:38:38Z

Yes, the finally makes it so the test always removes the file created
regardless of if the test errors or not. I can move it to teardown I guess,
I just hate having test specific things in teardown instead of in the test.

On Tue, Feb 10, 2015 at 4:14 PM, Yoshiki Vázquez Baeza <
notifications@github.com> wrote:

This PR looks ok but I'm not super familiar with this codebase so it would
be nice if someone else could review it in depth.

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

ElDeveloper · 2015-02-11T05:05:28Z

Ah got it. IMHO it's clearer if it's part of the teardown method (maybe
because that's what I'm used to see) than as an inline try/except block.
teardown methods are supposed to be dealing with these kind of things so
it's really up to you.

On (Feb-10-15|20:38), Joshua Shorenstein wrote:

Yes, the finally makes it so the test always removes the file created
regardless of if the test errors or not. I can move it to teardown I guess,
I just hate having test specific things in teardown instead of in the test.

On Tue, Feb 10, 2015 at 4:14 PM, Yoshiki Vázquez Baeza <
notifications@github.com> wrote:

This PR looks ok but I'm not super familiar with this codebase so it would
be nice if someone else could review it in depth.

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

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

coveralls · 2015-02-11T13:19:04Z

Coverage increased (+0.0%) to 78.61% when pulling 7e13efe on squirrelo:issue-835 into 32d6d8e on biocore:master.

antgonza · 2015-02-11T15:22:50Z

qiita_db/util.py

@@ -608,8 +608,8 @@ def get_mountpoint(mount_type, conn_handler=None, retrieve_all=False):
            "SELECT data_directory_id, mountpoint, subdirectory FROM "
            "qiita.data_directory WHERE data_type='%s' and active=true"
            % mount_type)]
-
-    return [(d, join(get_db_files_base_dir(), m, s)) for d, m, s in result]
+    basedir = get_db_files_base_dir()


I do not understand this change, is it required or is for clarity?

The function does an SQL select every time it's called, so this makes it happen 1 time instead of N times.

Excellent point! Thanks for catching that.

Should this use get_db_files_base_dir or should we move to the get_mountpoint function that @antgonza added a while ago?

This line is within the get_mountpoint function ....

Ooops, I just saw that this is actually the get_mountpoint function... 😅😇

josenavas · 2015-02-11T16:27:00Z

Few comments @squirrelo

wasade · 2015-02-11T20:37:37Z

qiita_db/support_files/patches/python_patches/14.py

+    'SELECT f.* from qiita.filepath f JOIN qiita.analysis_filepath afp ON '
+    'f.filepath_id = afp.filepath_id')
+for filepath in filepaths:
+    filename = basename(filepath['filepath'])


Just to verify: these files are still relative to a unique directory associated with a study or a meta-analysis, correct?

antgonza · 2015-02-12T15:20:25Z

Pull from upstream master.

squirrelo · 2015-02-12T16:13:42Z

ready for review again.

antgonza · 2015-02-12T16:50:52Z

The patch is not working in my machine because the mountpoint has an ending / and the file path doesn't

Traceback (most recent call last):
  File "qiita_db/support_files/patches/python_patches/14.py", line 23, in <module>
    mp_id = mountpoints[dirname(filepath['filepath'])]
KeyError: '/Users/antoniog/Desktop/CurrentWork/largeFiles/qiita/base_dir/analysis'

and I have these values in filepath, mountpoints

[308L, '/Users/antoniog/Desktop/CurrentWork/largeFiles/qiita/base_dir/analysis/1_analysis_mapping.txt', 9L, '833932644', 1L, 1L]
{'/Users/antoniog/Desktop/CurrentWork/largeFiles/qiita/base_dir/analysis/': 1L}

wasade · 2015-02-12T16:56:20Z

Should the mount points have a trailing '/'? That technically refers to the contents of the path, not that path

antgonza · 2015-02-12T17:02:55Z

Got it but the current configuration allows you to have either so validating both cases or not allowing one in the configuration should be the way to go.

coveralls · 2015-02-12T18:13:02Z

Coverage decreased (-0.02%) to 78.58% when pulling 77b311c on squirrelo:issue-835 into 1ca033a on biocore:master.

antgonza · 2015-02-12T19:04:40Z

👍

antgonza · 2015-02-13T13:52:05Z

@squirrelo conflicts, @ElDeveloper could you merge after conflicts are solved? Thanks.

coveralls · 2015-02-13T18:16:46Z

Coverage decreased (-0.02%) to 78.48% when pulling 5a38e03 on squirrelo:issue-835 into 559556e on biocore:master.

Issue 835

squirrelo added 4 commits February 10, 2015 13:54

add filename, not fullpath. Fix qiita-spots#835

eca489f

add test for _add_files function

a679a01

add patch to fix filepaths currently in database

9e91f54

flake8

ab3ac5c

ElDeveloper reviewed Feb 11, 2015
View reviewed changes

squirrelo added 2 commits February 11, 2015 05:00

teardown

645b306

flake8

7e13efe

antgonza reviewed Feb 11, 2015
View reviewed changes

wasade reviewed Feb 11, 2015
View reviewed changes

squirrelo added 2 commits February 11, 2015 15:04

use insert_filepaths to add files

3f8e1b7

update patches

60979f7

squirrelo added 2 commits February 12, 2015 07:26

Merge branch 'master' of https://github.com/biocore/qiita into issue-835

5b2532c

flake8

d620a32

strip trailing slash if it exists

77b311c

merge upstream/master

5a38e03

ElDeveloper added a commit that referenced this pull request Feb 13, 2015

Merge pull request #845 from squirrelo/issue-835

17da9c2

Issue 835

ElDeveloper merged commit 17da9c2 into qiita-spots:master Feb 13, 2015

Issue 835 #845

Issue 835 #845

Uh oh!

Conversation

squirrelo commented Feb 10, 2015

Uh oh!

ElDeveloper commented Feb 10, 2015

Uh oh!

squirrelo commented Feb 10, 2015

Uh oh!

ElDeveloper commented Feb 10, 2015

Uh oh!

coveralls commented Feb 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElDeveloper commented Feb 11, 2015

Uh oh!

squirrelo commented Feb 11, 2015

Uh oh!

ElDeveloper commented Feb 11, 2015

Uh oh!

coveralls commented Feb 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Feb 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antgonza commented Feb 12, 2015

Uh oh!

squirrelo commented Feb 12, 2015

Uh oh!

antgonza commented Feb 12, 2015

Uh oh!

wasade commented Feb 12, 2015

Uh oh!

antgonza commented Feb 12, 2015 via email

Uh oh!

coveralls commented Feb 12, 2015

Uh oh!

antgonza commented Feb 12, 2015

Uh oh!

antgonza commented Feb 13, 2015

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

Uh oh!