Change in vdsm[master]: network: add hosttail_join to address.py

ishaby at redhat.com ishaby at redhat.com
Thu Mar 31 09:32:28 UTC 2016


Idan Shaby has posted comments on this change.

Change subject: network: add hosttail_join to address.py
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/55473/1/lib/vdsm/common/network/address.py
File lib/vdsm/common/network/address.py:

PS1, Line 56: '
> We usually use single quotes
Done


https://gerrit.ovirt.org/#/c/55473/1/tests/common/network_test.py
File tests/common/network_test.py:

Line 67:     def test_hosttail_ipv6_no_brackets_returns_garbage(self):
Line 68:         self.assertNotEqual(('2001::1', '4321'),
Line 69:                             ipaddress.hosttail_split('2001::1:4321'))
Line 70: 
Line 71:     @permutations([
> Please use the convention above for tests and avoid the permutations, it is
IMHO, permutations are readable, clear and were meant exactly for these kind of tests.
I don't think that duplicating the code six times (as the number of permutations) and make it ~ three times bigger will make it more readable, it will just make it longer.

The third test proves that even if it's not an IPv6 legal address, the method still does what it claims, and thus proves that we don't wrap only IPv6 addresses with "[]", but any string that contains a colon. If you think that this permutation is irrelevant, I don't mind to remove it.

There are plenty of places where we use permutations in vdsm.
Please consider to ack this patch and not delaying the patches that depend on it. You can always send a patch that transforms this code to your style, if you like. I will not be against it.
Line 72:         # (host, tail, expected)
Line 73:         ("server", "/", "server:/"),
Line 74:         ("server", "/path", "server:/path"),
Line 75:         ("ser:ver", "/path", "[ser:ver]:/path"),


Line 68:         self.assertNotEqual(('2001::1', '4321'),
Line 69:                             ipaddress.hosttail_split('2001::1:4321'))
Line 70: 
Line 71:     @permutations([
Line 72:         # (host, tail, expected)
> expected_return_value is little too long,  if you use "expected", the test 
Done
Line 73:         ("server", "/", "server:/"),
Line 74:         ("server", "/path", "server:/path"),
Line 75:         ("ser:ver", "/path", "[ser:ver]:/path"),
Line 76:         ("12.34.56.78", "/path", "12.34.56.78:/path"),


-- 
To view, visit https://gerrit.ovirt.org/55473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0b778445117090072d64c89ccfd94aaf3c12fa3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list