Code review comment for lp://qastaging/~diegosarmentero/ubuntuone-client/search-filter

Revision history for this message
Mike McCracken (mikemc) wrote :

I don't really agree with replacing spaces with '.+'.

The old share_links_search does a substring search in just the
basename, while this matches the whole path, which I think is a good
idea.

However, I was expecting this to also be substring search, but you are
replacing spaces in the search string with '.+', which means the
behavior is a little unexpected wrt. spaces in the search text.

Here's a test to be added to test_fsm.py in FSMSearchTestCase that shows what I mean:

    def test_get_paths_by_pattern_with_spaces(self):
        """Test that spaces in a pattern are handled as expected."""
        mdid = 'id'
        path = '/my/path/to/a/file/deep/in/the/woods/called/a file'
        mdobj = {'server_hash': 'asdqwe123'}
        mdid2 = 'id2'
        path2 = '/my/path/to/anythingyouwant-to-defile'
        mdobj2 = {'server_hash': 'asdqwe456'}
        self.fsm._idx_path = {path: mdid, path2: mdid2}
        self.fsm.fs = {mdid: mdobj, mdid2: mdobj2}
        expected = [path]
        # expectations
        paths = "(%s|%s)" % ('/my/path/', '/home/')
        pattern = paths + ".*/.*%s.*$"
        keywords = '.+'.join(re.escape('a file').split('\\ '))
        print "keywords:", keywords
        search = pattern % keywords
        result = self.fsm.get_paths_by_pattern(search)
        print "result from get_paths_by_pattern:", result
        self.assertEqual(result, expected)

I would expect "a file" to not match "anythingyouwant-to-defile". It
matches both:

Here's what that test prints:
keywords: a.+file
searching through /my/path/to/a/file/deep/in/the/woods/called/a file id
match with p= /my/path/to/a/file/deep/in/the/woods/called/a file
searching through /my/path/to/anythingyouwant-to-defile id2
match with p= /my/path/to/anythingyouwant-to-defile
result from get_paths_by_pattern: ['/my/path/to/a/file/deep/in/the/woods/called/a file', '/my/path/to/anythingyouwant-to-defile']
Traceback (most recent call last):
  File "/Users/mmccrack/Documents/Canonical/Source/test-improve-buildout/scripts/devsetup/parts/ubuntuone-client/tests/syncdaemon/test_fsm.py", line 4361, in test_get_paths_by_pattern_with_spaces
    self.assertEqual(result, expected)
  File "/Users/mmccrack/Documents/Canonical/Source/test-improve-buildout/scripts/devsetup/eggs/Twisted-11.1.0-py2.7-macosx-10.7-x86_64.egg/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = ['/my/path/to/a/file/deep/in/the/woods/called/a file',
 '/my/path/to/anythingyouwant-to-defile']
b = ['/my/path/to/a/file/deep/in/the/woods/called/a file']

review: Needs Fixing

« Back to merge proposal