Vrouter fixes pylint

[romain-dartigues]

In my dreams, all Python code pass through pylint and must validate before a merge.

This merge request is becoming quite big, maybe too much; I'd like to have the owners opinion.

It follow the previous pull requests:

• systemvm - Python code cleanup #1575
  • Vrouter fixes pylint CLDIN/cloudstack#2
    • Fixes for VirtualRouters in Basic Networking, especially with mutliple ranges in VLANs #1547

[The-Loeki]

kudos for all the hard work! Most of this should've been done a long time ago as pylint is one of the first firewalls of code quality ensurance.

Side notes:

• no fan of %s syntax; Python recommeds .format(), which should make the code a lot more elegant.
• Strings oughta be single-quoted unless double quotes are necessary

e.g.:

-    self.fw.append(
-        ["nat", "",
-         "-I PREROUTING -i ppp+ -m tcp --dport 53 -j DNAT --to-destination %s" %
-         local_ip
-         ]
-    )
+    self.fw.append([
+        'nat', '',
+        '-I PREROUTING -i ppp+ -m tcp --dport 53 -j DNAT --to-destination {}'.format(
+            local_ip
+        )])

[romain-dartigues]

Hi @The-Loeki, and thank you for your prompt comment!

For your side notes:

• this is mostly a matter of personal preference, I don't care what you use but here are my argument for the %s:
  • more portable than the .format() (Python 2.4 → Python 3)
  • coherent with logging methods; ie: logging.debug('foo {}', 'bar') = TypeError, the correct syntax is logging.debug('foo %s', 'bar')
• I also prefer single quotes, especially because double quotes are never "nessary" (in Python single and double are equivalent)

I didn't want to change "too much" (well, I think my patch is already too much; we should focus on documentation after that); if I did replace all double quote with single, for example, I would break git blame way too much for my taste.

But I'm totally willing to put some more work for conformity, if there is a will for it upstream ;^)

[The-Loeki]

Hi :) sorry I was unable to process your request to merge your PR with mine, as you can probably tell by now this would've oversized & muddied both PR's ;)
Depending on the uptake of the CS team here I'm quite willing to help you with it, because as I mentioned IMHO this should've been done from the start on.

So the .format() syntax is less compatible as you mention. It is however only incompatible with long-deprecated Python versions and CS SysVM's are compatible so that's not really an issue.
While of course it is a matter of personal taste, the official message from Python pplz however is that .format() is intended to replace the %s formatting: https://docs.python.org/2/library/stdtypes.html#str.format , which is why I and the other OSS projects I work on prefer it.

The logging one is an excellent remark though, wading through the docs I notice that they've added the .format() syntax as per 3.2: https://docs.python.org/3/howto/logging-cookbook.html#use-of-alternative-formatting-styles but of course only for the Formatter, not for the actual log messages which as stated over there must be kept in % syntax for backwards compat

I'm not sure from the top of my head which version the SysVM's are using, but if it's not Py3 it oughta be (here comes the next big-ass PR ;) )

[rohityadavcloud]

@romain-dartigues can you rebase against latest master and push -f?

[romain-dartigues]

Done.
Le me know what you think about it (I don't think Jenkins failed because of my edits?).

[rohityadavcloud]

@romain-dartigues can you fix the conflicts thanks

[rohityadavcloud]

Thanks @romain-dartigues for your PR, I've included similar changes in PR #2211 and added pylint/pep checks to Travis. Due to duplicate nature of work and that this PR has not received much activity in last 1+ year I'll close this. Many thanks again for your hard work and initiative.