| 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) *  |
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) * |
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) *  |
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 that the patch looked good was over one year ago. |
|
|
| msg257122 - (view) |
Author: R. David Murray (r.david.murray) * |
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) * |
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) * |
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) * |
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 |
|
|