classification
Title: There is a duplicate function in Lib/test/test_pathlib.py
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: BreamoreBoy, NAVNEET.SUMAN, ezio.melotti, jesstess, josephgordon, pitrou, r.david.murray, vajrasky
Priority: normal Keywords: easy, patch

Created on 2013-12-03 08:30 by vajrasky, last changed 2015-12-29 12:41 by NAVNEET.SUMAN. This issue is now closed.

Files
File name Uploaded Description Edit
remover_duplicate_function.patch NAVNEET.SUMAN, 2014-02-25 22:13 review
Messages (14)
msg205082 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-12-03 08:30
Here it is (Lib/test/test_pathlib.py, line 1240):

    def _check_resolve_relative(self, p, expected):
        q = p.resolve()
        self.assertEqual(q, expected)

    def _check_resolve_absolute(self, p, expected):
        q = p.resolve()
        self.assertEqual(q, expected)
msg205089 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-03 09:02
Well, it's not really a duplicate function (the code is the same, but the intent is different). I'm not sure it's worth deduplicating.
msg205094 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-12-03 09:21
These functions are only being used in test_resolve_common.

    def test_resolve_common(self):
        P = self.cls
        p = P(BASE, 'foo')
        with self.assertRaises(OSError) as cm:
            p.resolve()
        self.assertEqual(cm.exception.errno, errno.ENOENT)
        # These are all relative symlinks
        p = P(BASE, 'dirB', 'fileB')
        self._check_resolve_relative(p, p)
        p = P(BASE, 'linkA')
        self._check_resolve_relative(p, P(BASE, 'fileA'))
        p = P(BASE, 'dirA', 'linkC', 'fileB')
        self._check_resolve_relative(p, P(BASE, 'dirB', 'fileB'))
        p = P(BASE, 'dirB', 'linkD', 'fileB')
        self._check_resolve_relative(p, P(BASE, 'dirB', 'fileB'))
        # Now create absolute symlinks
        d = tempfile.mkdtemp(suffix='-dirD')
        self.addCleanup(shutil.rmtree, d)
        os.symlink(os.path.join(d), join('dirA', 'linkX'))
        os.symlink(join('dirB'), os.path.join(d, 'linkY'))
        p = P(BASE, 'dirA', 'linkX', 'linkY', 'fileB')
        self._check_resolve_absolute(p, P(BASE, 'dirB', 'fileB'))

Why not just combine them to _check_resolve to avoid confusion? It did confuse me. I was not sure whether I had to check the absolute link differently than the relative link.
msg211277 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-02-15 14:39
I agree that two functions with different names and same code seem confusing.
I think replacing them with _check_resolve (and possibly add _check_resolve_relative = _check_resolve_absolute = _check_resolve) would be OK.
Antoine, if you disagree, feel free to close this issue (or suggest something else).
msg212226 - (view) Author: NAVNEET SUMAN (NAVNEET.SUMAN) * Date: 2014-02-25 22:13
made patch according to Ezio Melotti
msg212966 - (view) Author: Jessica McKellar (jesstess) * (Python triager) Date: 2014-03-09 17:51
Thanks for the patch, NAVNEET.SUMAN!

The patch implements ezio.melotti's proposal and applies cleanly without test regressions for me locally.

=> patch review
msg220394 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-12 22:48
ping.
msg236996 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-03-01 23:51
Pang :(
msg242859 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-05-10 15:21
Can we have a formal commit review please as Jessica's comment in msg212966 that the patch looked good was over one year ago.
msg257122 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-28 18:19
Maybe add a comment as to why this is done?  (I'm not sure I understand why.)
msg257123 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2015-12-28 18:22
As far as I understand the two functions exist only for readability (i.e. the intention is to check for relative and absolute resolution respectively), but since they share the same implementation, the patch defines a single function and uses it for both instead of duplicating the code.
msg257125 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-28 18:28
So maybe "# we can use the same method to check for both absolute and relative resolution"?
msg257131 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2015-12-28 21:51
Applied to default in 1472c08d9c23.
Thanks Navneet for the patch!
msg257175 - (view) Author: NAVNEET SUMAN (NAVNEET.SUMAN) * Date: 2015-12-29 12:41
Thanks.. Finally after two years the patch got submitted. :D
History
Date User Action Args
2015-12-29 12:41:55NAVNEET.SUMANsetmessages: + msg257175
2015-12-28 21:51:55ezio.melottisetstatus: open -> closed

assignee: pitrou -> ezio.melotti
versions: + Python 3.6, - Python 3.5
messages: + msg257131
type: behavior -> enhancement
resolution: fixed
stage: patch review -> resolved
2015-12-28 18:28:18r.david.murraysetassignee: ezio.melotti -> pitrou
stage: commit review -> patch review
messages: + msg257125
versions: + Python 3.5, - Python 3.6
2015-12-28 18:22:26ezio.melottisetassignee: pitrou -> ezio.melotti
stage: patch review -> commit review
messages: + msg257123
versions: + Python 3.6, - Python 3.5
2015-12-28 18:19:37r.david.murraysetnosy: + r.david.murray
messages: + msg257122
2015-12-22 08:57:08josephgordonsetnosy: + josephgordon
2015-05-10 15:21:43BreamoreBoysetmessages: + msg242859
2015-03-01 23:51:20BreamoreBoysetmessages: + msg236996
2014-06-12 22:48:51BreamoreBoysetnosy: + BreamoreBoy
messages: + msg220394
2014-03-18 04:34:53eric.araujosetassignee: pitrou
2014-03-09 17:51:52jesstesssetnosy: + jesstess

messages: + msg212966
stage: needs patch -> patch review
2014-02-25 22:13:55NAVNEET.SUMANsetfiles: + remover_duplicate_function.patch

nosy: + NAVNEET.SUMAN
messages: + msg212226

keywords: + patch
2014-02-15 14:39:22ezio.melottisetversions: + Python 3.5, - Python 3.4
nosy: + ezio.melotti

messages: + msg211277

keywords: + easy
stage: needs patch
2013-12-03 09:21:18vajraskysetmessages: + msg205094
2013-12-03 09:02:26pitrousetmessages: + msg205089
2013-12-03 08:30:49vajraskycreate