On Fri, Feb 22, 2013 at 4:53 PM, Michael Hanselmann <[email protected]>wrote:
> - Commit 4a90bd4 contained a rather large number of style violations:
> - Indentation/sequence formatting
> - Wrapping of long lines
> - Docstrings whose first line was wrapped
> - A stray backslash in a docstring
> - opcodes: Don't check for list or tuple. None of the other opcodes does
> it explicitely. As long as the length and the items match the value is
> accepted.
>
I did it because I found such a check in _BuildJobDepCheck.
It's not an OpCode, though, so, OK.
> - server/noded: “if variable” doesn't test for None, but False
> ---
> lib/opcodes.py | 10 ++++------
> lib/rapi/rlib2.py | 9 ++++-----
> lib/server/noded.py | 12 +++++++-----
> test/py/ganeti.rapi.rlib2_unittest.py | 6 ++----
> 4 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/lib/opcodes.py b/lib/opcodes.py
> index 43f4973..59af3f6 100644
> --- a/lib/opcodes.py
> +++ b/lib/opcodes.py
> @@ -1452,13 +1452,11 @@ class OpInstanceReboot(OpCode):
> ("reboot_type", ht.NoDefault, ht.TElemOf(constants.REBOOT_TYPES),
> "How to reboot instance"),
> ("reason", (constants.INSTANCE_REASON_SOURCE_UNKNOWN, None),
> - ht.TAnd(ht.TOr(ht.TList, ht.TTuple),
> - ht.TIsLength(2),
> + ht.TAnd(ht.TIsLength(2),
> ht.TItems([
> - ht.TElemOf(constants.INSTANCE_REASON_SOURCES),
> - ht.TMaybeString,
> - ])
> - ),
> + ht.TElemOf(constants.INSTANCE_REASON_SOURCES),
> + ht.TMaybeString,
> + ])),
>
Ok, I got the intendation wrong. I see. It should not be a multiple of two
spaces, but just two spaces from the previous indentation level, even if,
as in this case, it becomes something that is not a multiple of two.
> "The reason why the reboot is happening"),
> ]
> OP_RESULT = ht.TNone
> diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py
> index 89a99c5..a475151 100644
> --- a/lib/rapi/rlib2.py
> +++ b/lib/rapi/rlib2.py
> @@ -1040,11 +1040,10 @@ class
> R_2_instances_name_reboot(baserlib.OpcodeResource):
> self.queryargs.get("type", [constants.INSTANCE_REBOOT_HARD])[0],
> "ignore_secondaries":
> bool(self._checkIntVariable("ignore_secondaries")),
> "dry_run": self.dryRun(),
> - "reason": (
> - constants.INSTANCE_REASON_SOURCE_RAPI,
> - self._checkStringVariable("reason_text",
> -
> default=constants.INSTANCE_REASON_REBOOT),
> - )
> + "reason":
> + (constants.INSTANCE_REASON_SOURCE_RAPI,
> + self._checkStringVariable("reason_text",
> +
> default=constants.INSTANCE_REASON_REBOOT)),
> })
>
The only thing similar to this that I found in the style guide was:
var = {
"key": func(),
"otherkey": None,
}
I see it's not the exact same thing, but it was the only place where
something starting with a parenthesis was divided in more lines, and the
parenthesis itself remains on the first line, alone.
Maybe we should add an example of this in the style guide for future
reference?
>
> diff --git a/lib/server/noded.py b/lib/server/noded.py
> index aa9243f..96ad1ac 100644
> --- a/lib/server/noded.py
> +++ b/lib/server/noded.py
> @@ -113,12 +113,14 @@ def _DecodeImportExportIO(ieio, ieioargs):
>
>
> def _DefaultAlternative(value, default):
> - """Returns the given value, unless it is None. In that case, returns a
> - default alternative.
> + """Returns value or, if evaluating to False, a default value.
>
You wrote this in the first review of the patch and I forgot to fix it.
Sorry.
But I think it's better to clearly specify in the style guide that the
first sentence cannot be longer than one line.
I say all of this not to complain about the things you correctly pointed
out, but because I wrote this patch with the style guide constantly open,
and if I managed to get so many things wrong I might probably try to
improve it a little bit, if you agree.
Thanks,
Michele
