General¶

Nice one. I like the fact that you laid out the comments more-or-less according to the guidelines.

CADImport.py main() function¶

The stuff at the bottom

geometry1 = CADImport("fastradModel.xml", "GDML2G4MICE.xsl", "", "OUTPUTFILE.txt")
geometry2 = CADImport("fastradModel.xml", "GDML2G4MICE.xsl")
geometry3 = CADImport("fastradModel.xml", "Merge.xsl", "FieldInfoTest.xml")

geometry1.XSLTParse()
geometry3.AppendMerge()

You should put this in a main() function like

def main():
  geometry1 = CADImport("fastradModel.xml", "GDML2G4MICE.xsl", "", "OUTPUTFILE.txt")
  geometry2 = CADImport("fastradModel.xml", "GDML2G4MICE.xsl")
  geometry3 = CADImport("fastradModel.xml", "Merge.xsl", "FieldInfoTest.xml")

  geometry1.XSLTParse()
  geometry3.AppendMerge()

Then add code like:

if name == '__main__':
main()

That way, if I do

python CADimport.py

The code in main runs. If I do

import CADimport

Comments¶

What I think is missing is a basic overview of what the class does. Coming to it with half a memory of our conversation I have a vague feeling that this does a merge between two files, but you really need to explain this to me. Most people haven't heard of xslt, you should at least explain what this does in a sentence. This can either go at the top of the class or at the top of the file (module).

        This method initialises the class and takes 7 parameters. The later 4 parameters are optional
        but if one of the latter needs to be initialised a blank "" needs to be placed to set preceeding 
        params to None. All arguments should be file names or paths.

        @param xmlin1,   first xml argument
        @param xsl,      xslt argument
        @param xmlin2,   second xml argument, Default None
        @param output,   output argument, Default None
        @param mergein,  template XSLT for append, Default Merge.xsl.in
        @param mergeout, destination for edited XSLT for append, Default Merge.xsl

So, typically the developer can see the arguments that are optional so you don't need to specify this. What you do need to specify is that xmlin1 is a file path to an xml filename that will be merged with xmlin2 or whatever...

Tests¶

Testing is a bit of a pain in code that is essentially making an interface to an external library. Tests I would like to see are like:

AssertMerge¶

1. multiple EDIT commands on same line
2. EDIT commands on different lines
3. Error message if EDIT not found

XSLTParse¶

Write a short file to $MAUS_ROOT_DIR/tmp, only has to be a couple of lines long and check that it gets parsed correctly, or appropriate error if the file doesn't exist etc.

I don't see how the merge code works for when we have two files (maybe this code doesn't exist yet?)

• src/core/CADImport.py
  • In particular the constructor as I'm not entirely sure its the best way of instatianting the class object

• tests/unit/test_CADImport.py
  • just too see if these tests are ok

and lastly ive added a couple of .bash scripts in the third party directory to build the libraries needed for the code above and just want to check they look ok?

Thanks

Basic workflow¶

1. calls GDMLFormatter.formatter to manipulate paths so that they are correct
2. calls GDMLtoCDB.gdmltocdb to upload to cdb
3. calls packer to pack into a zip file for local cacheing

1. GDMLtoCDB.downloader pulls data off the configdb
2. CADImport.CADImport does the merge with an external geometry file containing e.g. field maps

Comments¶

• STYLE argparse stuff is nice - but better to use src/common_py/Configuration.py and associated functions so the interface is common. This will get argparse interface some time.
• STYLE uploadGeometry.py - please use consistent style for file name capitalisation

• BUG - hard coded file names should be changeable at runtime (Configuration file)
• STYLE "obj" - better variable name please

• BUG - again server url should be soft coded
• STYLE - notes field not commented
• STYLE - change end of file string to be more formal. Probably should be an xml comment
• STYLE - no colons (compatibility with smb), no spaces (sanity) in filenames

• BUG - mergout variable name clearly wrong. Should have been picked up in tests.

Putting a new code review in which will cover this file.