On Wed, Feb 13, 2013 at 11:23 AM, Michael Hanselmann <[email protected]>wrote:
> 2013/2/12 Michele Tartara <[email protected]>: > > lib/backend.py | 46 > ++++++++++++++++++++++++++++++++++++++++ > > lib/cli.py | 5 +++++ > > lib/constants.py | 9 ++++++++ > > lib/pathutils.py | 16 ++++++++++++++ > > lib/tools/ensure_dirs.py | 2 ++ > > src/Ganeti/OpParams.hs | 10 +++++++++ > > src/Ganeti/Types.hs | 10 +++++++++ > > test/hs/Test/Ganeti/OpCodes.hs | 2 ++ > > Where are the unittests for the Python code? > Well, there is not much that has to be tested in here. In cli.py I added an option, and as far as I can tell no option is tested. Same goes for the constants added to constants.py, and the directory creation in ensure_dirs. I guess the only thing I missed that might need testing is the GetJson function of InstReason, in backend.py, and I'll add a test for that. > > > --- a/lib/backend.py > > +++ b/lib/backend.py > > +class InstReason(object): > > + """Class representing the reason for a change of state of a VM. > > + > > + It is used to allow an easy serialization of the reason, so that it > can be > > + written on a file. > > + > > + """ > > + > > + def __init__(self, text, source): > > There should not be an empty line before the first method (or member > in general) in a class. > Ok. > > > + """Initialize the class with all the required values. > > […] > > + def GetJson(self): > > + """Get the JSON representation of the InstReason > > + > > + @rtype: string > > + @return : The JSON representation of the object. > > + > > + """ > > + fields = ["text", "source"] > > + as_dict = dict((field, getattr(self, field)) for field in fields) > > Why don't you just write it as follows? > > return serializer.DumpJson(dict(text=self.text, source=self.source)) Initially it was a little bit different, and with more fields, so written like this it was more concise. But now it does not make particularly sense anymore, so I'll change it as you suggest. > > > + return serializer.DumpJson(as_dict) > > […] > > > --- a/lib/pathutils.py > > +++ b/lib/pathutils.py > > +def GetInstReasonFilename(instance_name): > > + """Returns the full path for the file name containing the reason of > the > > + last status change of an instance. > > The first line should not be wrapped. Put the rest in a paragraph. > Ok. > > > + > > + @type instance_name: string > > + @param instance_name: The name of the instance. > > No punctuation, please (might apply elsewhere too). > I did it like this because I've found it in a similar docstring in some part of the code. Unfortunately I don't remember where, but if I stumble on it again, I'll fix that as well. > > > + @rtype: string > > + @return: The path of the file. > > See above. > Ok. > > + > > + """ > > + return os.path.join(INSTANCE_REASON_DIR, instance_name) > > Now that I see this, I think you should put this function into > backend.py or utils/io.py. The reason is that you should use > utils.PathJoin. “os.path.join”' behaviour when an argument is absolute > or contains slashes (os.path.join("/tmp", "/bin/ls")) is undesired. If > someone manages to inject an instance name with a slash the code might > end up overwriting a file outside of INSTANCE_REASON_DIR. > Ok, I'll put it in backend.py. But then, why is GetLogFilename in pathutils.py? Is it because it does not receive parameters from the command line (but just from the source code) and so there is no security risk? (I'm just trying to understand where and why should I put other similar functions in the future) Thanks, Michele
